Re: [RFC/PATCH] recognize pathspec magic without "--" disambiguation

From: Junio C Hamano <gitster@pobox.com>
Date: May 26, 2017, 02:13 UTC
Message-ID: <xmqq4lw8cql8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170525152739.t63dbsq2dojy2y2h@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> But let's consider related invocations and whether we're
> making them better or worse:
>
>    - git log :/foo
>       (when "foo" matches a commit message)
>
>       This one should remain the same. Like the existing
>       wildcard rule, we're touching only verify_filename(),
>       not verify_non_filename(). So cases that _do_ resolve
>       as a rev will continue to do so.
>
>    - git log :^foo
>       (when "^foo" exists in your index)
>
>       The same logic applies; it would continue to work. And
>       ditto for any other weird filenames in your index like
>       "(attr)foo".

"git show :$path" where $path happens to be "^foo" would grab the contents of the $path out of the index and I think that is what you meant, but use of "git log" in the example made me scratch my head greatly.

Show 11 quoted lines

>    - git log :/foo
>       (when "foo" does _not_ match a commit message)
>	...
>       This same downside actually exists currently when you
>       have an asterisk in your regex. E.g.,
>
>         git log :/foo.*bar
>
>       will be treated as a pathspec (if it doesn't match a
>       commit message) due to the wildcard matching in
>       28fcc0b71.

In other words, we are not making things worse?

Show 20 quoted lines

> I wrote all the above to try to convince myself that this
> doesn't have any serious regression cases. And I think I
> did.
>
> But I actually we could make the rules in alternative (2)
> above work. check_filename() would ask the pathspec code to
> parse each argument and get one of three results:
>
>   1. it's not pathspec magic; treat it like a filename
>      (e.g., just "foo", or even bogus stuff like ":%foo")
>
>   2. it is pathspec magic, and here is the matching filename
>      that ought to exist (e.g., "foo" from ":^foo" or
>      ":(exclude)foo")
>
>   3. it is pathspec magic, but there's no matching filename.
>      Assume it's a pathspec (e.g., "(attr)foo").
>
> I'm on the fence on whether it's worth the trouble versus
> the simple rule implemented by this patch.

Unlike "git log builtin-checkout.c" that errors out (only because there is no such file in the checkout of the current version) and makes its solution obvious to the users, this change has the risk of silently accepting an ambiguous input and computing result that is different from what the user intended to. So I am not sure.

As you pointedout, ":/" especially does look like a likely point of failure, in that both "this is path at the top" pathspec magic and "the commit with this string" are not something that we can say with confidence that are rarely used because they are so esoteric.

As to "is it OK to build a rule that we cannot explain easily?", I think it is OK to say "if it is not a rev, and if it is not a pathname in the current working tree, you must disambiguate, but Git helps by guessing in some cases---if you want to have more control (e.g. you are a script), explicitly disambiguate and you'd be OK", and leave the "some cases" vague, as long as we are only making reasonably conservative guesses.