Bug in pathspec handling (in conjunction with submodules)
- Date: Sun, 26 Nov 2017 03:15:56 +0100 (CET)
- From: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
- Subject: Bug in pathspec handling (in conjunction with submodules)
Hi Duy & Brandon,
in 74ed43711fd (grep: enable recurse-submodules to work on <tree> objects,
2016-12-16), the do_match() function in tree-walk.c was changed so that it
can recurse across submodule boundaries.
However, there is a bug, and I *think* there may be two bugs actually. Or
First of all, here is an MCVE that I distilled from
git init repo
git init submodule
git -C submodule commit -m initial --allow-empty
git add "[bracket]"
git commit -m bracket
git add submodule
git commit -m submodule
git rev-list HEAD -- "[bracket]"
Nothing fancy, just adding a file with brackets in the name, then a
submodule, then showing the commit history filtered by the funny file
However, the log prints *both* commits. Clearly the submodule commit
should *not* be shown.
Now, how does this all happen?
Since the pathspec contains brackets, parse_pathspec() marks it as
containing wildcards and sets nowildcard_len to 0.
Now, note that [bracket] *is* a wildcard expression: it should only match
a single character that is one of a, b, c, e, k, r or t.
I think this is the first bug: `git rev-list` should not even match the
commit that adds the file [bracket] because its file name does not match
that expression. From where I sit, it would appear that f1a2ddbbc2d
(tree_entry_interesting(): optimize wildcard matching when base is
matched, 2010-12-15) simply added the fnmatch() code without disabling the
literal match_entry() code when the pathspec contains a pattern.
But it does not stop there: there is *another* bug which causes the
pattern to somehow match the submodule. I *guess* the idea of
was to allow a pattern like *.c to match files in a submodule, but the
pattern [bracket] should not match any file in submodule/. I think that
that code needs to be a little bit more careful to try to match the
submodule's name against the pattern (it seems to interpret nowildcard_len
== 0 to mean that the wildcard is `*`).
However, the commit introducing that code wanted to teach *grep* (not
*rev-list*) a new trick, and it relies on the `recursive` flag of the
pathspec to be set.
And now it gets really interesting. Or confusing, depending on your mental
condition. This recursive flag of the pathspec is set in
ll_diff_tree_paths() (yep, changing the flag in the passed-in opt
structure... which I found a bit... unexpected, given the function name, I
would have been less surprised if that function only diff'ed the trees and
used the options without changing the options). That flag-change was
which is another patch that changed the tree diff machinery to accommodate
`git grep` (but maybe not really paying a lot of attention to the fact
that the same machinery is called repeatedly by the revision machinery,
I am really confused by this code mainly due to the fact that the term
"recursive" is pretty ambiguous in that context: does it refer to
directories/tree objects, or to submodules? I guess it is used for both
when there should be two flags so that rev-list can recurse over tree
objects but not submodules (unless told to do so).
The problem, of course, is that `git rev-list HEAD -- '[bracket]'` never
recurses into the submodule. And therefore, the promised "more accurate
matching [...] in the submodule" is never performed. And the commit adding
the submodule is never pruned.
Since I am not really familiar with all that tree diff code (and as a
general rule to protect my mental health, I try my best to stay away from
submodules, too), but you two are, may I ask you gentle people to have a
closer look to fix those bugs?