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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2017, 22:53 UTC
Message-ID: <xmqq1stszxn4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACBZZX5P3eWxF0qMoi4u+Suct61PXP-hS+gd0s7b+hmMvJpS=w@mail.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Then "approxidate is case insensitive so others should too" argument does not hold water. Not that it has to, as I would prefer to see an honest justification for a feature. Among @{...} and ^{...}, approxidate is the only case insensitive one; if we are shooting for consistency we'd be making everything case sensitive, which is the opposite of what we want here.

Show 6 quoted lines

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

No, everything was a direct reply to you (I haven't even read Duy's message when I responded). Your patch that started the thread tried to justify it as making things consistent, and it was a direct response to it.

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

TL;DR

 - In proposed log message, only mention that we are doing this
   because holding SHIFT after typing "@{" makes 'Upstream' or
   'PUSH' easier to type than 'upstream' or 'push'.  Don't invoke
   the "case insensitivity for consistency" or "approximate is case
   insensitive" as a rationle.

 - In documentation, the current description that only mentions
   'push' and 'upstream' in lowercase is sufficient for signaling
   what is canonical.  Just adding a parenthetical note,
   e.g. ('push' and 'upstream' are accepted when spelled with
   uppercase and they mean the same thing no matter the case), after
   the current text finishes describing both of them is sufficient
   to help users who wonder why an otherwise undocumented @{PUSH}
   that they typed by mistake was accepted, and also help them when
   they see others write @{Push} in their scripts and wonder if it
   means something different from what they know, i.e. @{push}.

 - Do not allow upcasing ^{<type>}, at least for now.

and a longer version.

The reason why I care deeply about how a change justifies itself in the log message is because it can set the design principle for the future of the system. We shouldn't say "Anything inside @{...} should be case insensitive." to avoid limiting the design space unnecessarily for those who design new features @{/<pattern>} etc. in the future, for example.

Most parts of the system, not limited to @{} and ^{} syntax, are case sensitive, and there is no point repeating "this is case sensitive except..." all over the place.

However, if we make some part case insesitive without documenting, or make any "hidden" feature without documenting in general, it risks confusing the users in two possible ways:

 (1) the user may trigger an undocumented feature first by mistake,
     and will wonder why it worked as expected and will also wonder
     if that is guaranteed to work in the future;

 (2) the user may see the use of an undocumented feature, and would
     wonder if there is any difference in the behaviour with the
     documented counterpart.

A parenthetical note should be designed to help with the above two points.

Because we do not accept "cat-file -t <type>" and "hash-object -t <type>" in a case insensitive way, it is not a good idea to allow "^{COMMIT}", especially because it is not clear if allowing "cat-file -t COMMIT" is a good idea at all. We never restricted that the "unknown type" must be spelled in lowercase, and without such restriction, it is never safe to allow <type> to be case insensitive ("^{<type>}" is much less necessary these days anyway as "$OBJECT^0" and "$OBJECT:" are shorter and easier to type and can be used for most of the cases you may want to use "^{<type>}").