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

On Tue, Jan 3, 2017 at 11:55 PM, Jeff King [off-list ref] 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