Re: [PATCH] rev-parse: match @{u}, @{push} and ^{<type>} case-insensitively

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Mar 19, 2017 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
> > Change the revision parsing logic to match @{upstream}, @{u}, @{push},
> > ^{commit}, ^{tree} etc. case-insensitively. All of these cases
> > currently emit "unknown revision or path not in the working tree"
> > errors.
> >
> > This change makes them equivalent to their lower-case versions, and
> > consistent with other revision format specifications, e.g. 'master@{6
> > hours ago}', which is equivalent to 'master@{6 HoUrS aGo}'.
>
> Approxidate is not just case insensitive, but it takes random
> non-word characters (e.g. a dot and a slash in "@{4.minutes/}") that
> are not spaces at word boundaries, and I do not think you want to
> accept @{.Upstream.} for consistency.
>
> It is an odd-man-out and "consistency" with it is a nonsense
> justification.


I'm not suggesting that we make all the options accept garbage like
the date option in the name of consistency.

I found it helpful to make a table out of this. Key CI = Case
Insensitive (now)?, CIP = Case Insensitive Possible (without
ambiguities)?, AG = Accepts Garbage (.e.g. @{./.4.minutes./.})?

Before this patch:

|----------------+-----+------+-----|
| What?          | CI? | CIP? | AG? |
|----------------+-----+------+-----|
| sha1           | Y   | -    | N   |
| describeOutput | N   | N    | N   |
| refname        | N   | N    | N   |
| @{<date>}      | Y   | Y    | Y   |
| @{<n>}         | N/A | N/A  | N   |
| @{<n>}         | N/A | N/A  | N   |
| @{upstream}    | N   | Y    | N   |
| @{push}        | N   | Y    | N   |
| ^{<type>}      | N   | Y    | N   |
| ^{/regex}      | N   | N    | N   |
|----------------+-----+------+-----|

After:

|----------------+-----+------+-----|
| What?          | CI? | CIP? | AG? |
|----------------+-----+------+-----|
| sha1           | Y   | -    | N   |
| describeOutput | N   | N    | N   |
| refname        | N   | N    | N   |
| @{<date>}      | Y   | Y    | Y   |
| @{<n>}         | N/A | N/A  | N   |
| @{<n>}         | N/A | N/A  | N   |
| @{upstream}    | Y   | -    | N   |
| @{push}        | Y   | -    | N   |
| ^{<type>}      | Y   | -    | N   |
| ^{/regex}      | N   | N    | N   |
|----------------+-----+------+-----|

I.e. now we have 3x forms that could without any ambiguity be case
insensitive, this patch makes that so. We have one option that's very
loose about accepting garbage (@{<date>}). I don't see any reason to
try to pursue making the other options accept similar garbage.

>
> > The use-case for this is being able to hold the shift key down while
> > typing @{u} on certain keyboard layouts, which makes the sequence
> > easier to type, and reduces cases where git throws an error at the
> > user where it could do what he means instead.
>
> This, on the hand, is a sane justification that can be sympathized.


It's the reason I wrote the patch, and I'm not using consistency as
some argument for the change, I just had to take an inventory of all
these special forms and found out that these were the odd ones out in
the sense that everything else that can be case insensitive is.

>
> > The objection from Junio at the time[2] was that by lower-casing
> > {...}:
> >
> >     [The door would be closed on] allow[ing] @{/regexp} to find a
> >     reflog entry that matches the given pattern, and in such a use
> >     case we would certainly want to take the pattern in a case
> >     sensitive way.
> >
> > This appears to be an objection related to the code structure at the
> > time,...
>
> This objection, which is not about code structure but about design,
> still applies, I would think, if your justification is "consistency
> by making everything case-insensitive".
>
> Whoever is doing @{/<pattern>} cannot add the feature in a case
> sensitive way without violating the declaration you are making here:
> "everything inside @{...} is case-insensitive".

That's a quote from Duy's E-Mail. I don't think we should document
document anything like that, and my patch doesn't do that.

It is a legit question whether we document things as "unless otherwise
noted everything's case insensitive", and then list the exceptions, or
"unless otherwise noted everything's case sensitive", and then list
the exceptions. My patch does the former, Duy was suggesting the
latter.

I don't have any strong preference for either really, but neither
locks us into any future promises. It's just a matter of how the
current documentation phrases things.

>
> And if you extend that declaration to say "everything inside ^{...},
> too, is case-insensitive", I think it already is broken as I think
> "^{/<pattern>}" is case sensitive, by the way.

Yes, I agree that phrasing things like Duy suggested offhand in that
E-Mail would be broken.

> So don't pretend that this is about consistency.  You are making a
> choice for one class of strings that can go inside @{...} and the
> choice does not depend on the case sensitivity of different classes
> of strings that can go the same place.

I think this too is really just a reply to what Duy said...

> [...]
> I think "immediately after typing '{', you often have SHIFT
> pressed", even though it may sound lame, is a much better
> justification.  At least, it is an honest one.  And I do not mind
> too much if the way this feature is sold to the users were "these
> keywards inside @{...} can be spelled in any case: push, upstream.
> Type names in the peel-onion operator ^{<type>} can be too", not as
> a general rule but as special cases.  Unlike end-user supplied
> strings taken from an unbounded set (e.g. /<search patterns>), there
> is no strong reason to insist that a set of keywords taken from a
> limited vocabulary has to be spelled in one case, as long as it does
> not introduce ambiguity or limit our possible future.  It's not like
> we may want to keep the door open to make @{push} and @{PUSH} mean
> different things later.

*nod*

> Even in that case, however, I'd strongly prefer to spell all the
> examples in lowercase and declare that lowercase is the canonical
> spelling in our documentation.  What I want to avoid is to have
> three Git textbooks, that use @{UPSTREAM}, @{Upstream}, and
> @{upstream} in their samples and descriptions, and have the readers
> waste their time wondering, and waste our time by asking here, where
> the different preferences of the authors of these three books come
> from and which one the canonical way to spell it is.

So do you mean you'd like me to change the documentation to be more
like "While this is canonically lower case this form is case
insensitive so e.g. so-and-so also work" ?