On 01/04, Stefan Beller wrote:
> On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@xxxxxxxx> wrote:
> > But as this commit message needs to stand on its own, rather than as part of a
> > larger discussion thread, it might be worth expanding "one of the cases"
> > here. And talking about what's happening to the other cases.
> >
> > Like:
> >
> > This assertion triggered for cases where there wasn't a programming
> > bug, but just bogus input. In particular, if the user asks for a
> > pathspec that is inside a submodule, we shouldn't assert() or
> > die("BUG"); we should tell the user their request is bogus.
> >
> > We'll retain the assertion for non-submodule cases, though. We don't
> > know of any cases that would trigger this, but it _would_ be
> > indicative of a programming error, and we should catch it here.
>
> makes sense.
>
> >
> > or something. Writing the first paragraph made me wonder if a better
> > solution, though, would be to catch and complain about this case
> > earlier. IOW, this _is_ a programming bug, because we're violating some
> > assumption of the pathspec code. And whatever is putting that item into
> > the pathspec list is what should be fixed.
> >
> > I haven't looked closely enough to have a real opinion on that, though.
>
> Well I think you get different behavior with different flags enabled, i.e.
> the test provided is a cornercase (as "git add ." in the submodule should
> not yell at us IF PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
> were set, in my understanding of the code, so maybe the test rather adds
> a ./file/with/characters inside the submodule directory)
>
> I think a valid long term vision would be to have
>
> $ git -C submodule add file
> $ echo $?
> 0
>
> to behave the same as
>
> $ git add submodule/file
> advice/hint: adding file inside of a submodule
> $ echo $?
> 0
> $ git -c submodule.iKnowWhatIDo add submodule/anotherfile
> $ echo $?
> 0
>
> Brandon, who is refactoring the pathspec stuff currently may have
> an opinion if we could catch it earlier and still have beautiful code.
>
> Thanks,
> Stefan
>
> > Given the discussion, this comment seems funny now. Who cares about
> > "historically"? It should probably be something like:
> >
> > /*
> > * This case can be triggered by the user pointing us to a pathspec
> > * inside a submodule, which is an input error. Detect that here
> > * and complain, but fallback in the non-submodule case to a BUG,
> > * as we have no idea what would trigger that.
> > */
>
> Makes sense.
>
> >
> > -Peff
So there are really two different things going on in the pathspec code
with regards to submodules.
The case that this series is trying to solve is not because the user
provided a pathspec into a submodule, but rather they are executing in
the context of the submodule with bogus state. Typically this bogus
state has something to do with the submodule's .gitdir being blown away
(like in the last test (3/3) added in this patch). Because the
submodule doesn't have a .gitdir, it searches backward in the directory
hierarchy for a .gitdir and it happens to find the superproject's gitdir
and uses that as its own .gitdir. When this happens test 3/3 catches
that assert with the prefix being "sub/" and match being "sub" (since
the submodule slash was removed). The condition doesn't trigger when
you supply a pathspec of "./b/a" assuming you have a file 'a' in
directory 'b' inside the submodule, since the prefix would still be
"sub/" while the match string would be "sub/b/a". Coincidentally the
check that PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE does, does in fact
catch it (if using say the 'git add' command).
This leads me into the second case. If
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set, then any pathspec which
decends into a submodule will indeed be caught and cause and error (as
was happens in test 2/3 in this patch).
So in my opinion, the assert at the end of constructing a
pathspec object probably isn't the best place for determining if the
submodule's gitdir has been destroyed and instead it has fallen back to
its parent's gitdir. A check for something like this should happen much
sooner.
There are cases where it is advantages to be able to supply a pathspec
into a submodule without it erroring out (git grep --recurse-submodules
is one example). So right now the current method for not allowing a
pathspec into a submodule is to pass the STRIP_SUBMODULE_SLASH_EXPENSIVE
flag when creating the pathspec object.
--
Brandon Williams