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