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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

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

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