Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error

From: Stefan Beller <sbeller@google.com>
Date: Jan 4, 2017, 18:46 UTC
Message-ID: <CAGZ79kY3oVfn-xH4RQR9jMoKxoQUtF5HezY9HPUZGbx9KP-S5Q@mail.gmail.com>
In-Reply-To: <20170104075506.sa5oa5bheykswkwn@sigill.intra.peff.net>

On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

> 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.

Show 8 quoted lines

>
> 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

Show 9 quoted lines

> 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