Re: [PATCH] pathspec: only match across submodule boundaries when requested

[Brandon Williams <bmwill@google.com>]

On 11/29, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Tue, 28 Nov 2017, Brandon Williams wrote:
> 
> > Commit 74ed43711fd (grep: enable recurse-submodules to work on <tree>
> > objects, 2016-12-16) taught 'tree_entry_interesting()' to be able to
> > match across submodule boundaries in the presence of wildcards.  This is
> > done by performing literal matching up to the first wildcard and then
> > punting to the submodule itself to perform more accurate pattern
> > matching.  Instead of introducing a new flag to request this behavior,
> > commit 74ed43711fd overloaded the already existing 'recursive' flag in
> > 'struct pathspec' to request this behavior.
> > 
> > This leads to a bug where whenever any other caller has the 'recursive'
> > flag set as well as a pathspec with wildcards that all submodules will
> > be indicated as matches.  One simple example of this is:
> > 
> > 	git init repo
> > 	cd repo
> > 
> > 	git init submodule
> > 	git -C submodule commit -m initial --allow-empty
> > 
> > 	touch "[bracket]"
> > 	git add "[bracket]"
> > 	git commit -m bracket
> > 	git add submodule
> > 	git commit -m submodule
> > 
> > 	git rev-list HEAD -- "[bracket]"
> > 
> > Fix this by introducing the new flag 'recurse_submodules' in 'struct
> > pathspec' and using this flag to determine if matches should be allowed
> > to cross submodule boundaries.
> > 
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> 
> Could you also add something like
> 
> 	This fixes https://github.com/git-for-windows/git/issues/1371
> 
> at the end of the commit message, to keep a reference to the original bug
> report?

Yep! I can do that.

> 
> >  4 files changed, 22 insertions(+), 2 deletions(-)
> 
> Phew. That was much smaller than I expected.
> 
> > +test_expect_success 'tree_entry_interesting does not match past submodule boundaries' '
> > +	test_when_finished "rm -rf repo submodule" &&
> > +	git init submodule &&
> > +	test_commit -C submodule initial &&
> > +	git init repo &&
> > +	>"repo/[bracket]" &&
> > +	git -C repo add "[bracket]" &&
> > +	git -C repo commit -m bracket &&
> > +	git -C repo rev-list HEAD -- "[bracket]" >expect &&
> > +
> > +	git -C repo submodule add ../submodule &&
> > +	git -C repo commit -m submodule &&
> > +
> > +	git -C repo rev-list HEAD -- "[bracket]" >actual &&
> > +	test_cmp expect actual
> > +'
> 
> Nicely prepared for a new hash function, too (no explicit SHA-1).
> 
> I wonder, however, why we can't `git checkout -b bracket` and
> `test_when_finished "git checkout master"` and void those many `-C repo`
> options. But then, it is actually one of the shorter test cases, and
> pretty easy to understand.
> 
> However, I would still like to see `test_tick`s before those `git commit`
> calls, to make the commit names reproducible.

In v2 I added the calls to test_tick.  I've never used the function
myself so hopefully I used it correctly! :)

> 
> Thanks,
> Dscho

-- 
Brandon Williams