git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Conrad Irwin <conrad.irwin@gmail.com>,
	Sitaram Chamarty <sitaramc@gmail.com>,
	Michael J Gruber <git@drmicha.warpmail.net>,
	Nguyen Thai Ngoc Duy <pclouds@gmail.com>,
	Jeff King <peff@peff.net>, Richard Hansen <rhansen@bbn.com>,
	"Brian M . Carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH] rev-parse: match @{u}, @{push} and ^{<type>} case-insensitively
Date: Sun, 19 Mar 2017 15:53:35 -0700
Message-ID: <xmqq1stszxn4.fsf@gitster.mtv.corp.google.com> (raw)
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.

>> 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>}").

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-18 22:34 Ævar Arnfjörð Bjarmason
2017-03-19  9:19 ` Duy Nguyen
2017-03-19 10:04   ` Ævar Arnfjörð Bjarmason
2017-03-19 12:55 ` Junio C Hamano
2017-03-19 14:26   ` Ævar Arnfjörð Bjarmason
2017-03-19 22:53     ` Junio C Hamano [this message]
2017-03-26 12:16       ` [PATCH v2 0/3] rev-parse case insensitivity & @{p} synonym Ævar Arnfjörð Bjarmason
2017-03-26 12:16       ` [PATCH v2 1/3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively Ævar Arnfjörð Bjarmason
2017-03-27  0:27         ` Junio C Hamano
2017-03-27 11:16           ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2017-03-27 17:45             ` Junio C Hamano
2017-03-27 18:05               ` Ævar Arnfjörð Bjarmason
2017-03-26 12:16       ` [PATCH v2 2/3] rev-parse: add @{p} as a synonym for @{push} Ævar Arnfjörð Bjarmason
2017-03-27  2:53         ` Jeff King
2017-03-27  7:52           ` Ævar Arnfjörð Bjarmason
2017-03-26 12:16       ` [PATCH v2 3/3] rev-parse: match ^{<type>} case-insensitively Ævar Arnfjörð Bjarmason
2017-03-27  0:36         ` Junio C Hamano
2017-03-27  2:58           ` Jeff King
2017-03-27  5:39             ` Junio C Hamano
2017-03-27  7:11               ` Jeff King
2017-03-27  7:33             ` Ævar Arnfjörð Bjarmason
2017-03-21 19:19 ` [PATCH] rev-parse: match @{u}, @{push} and " Ævar Arnfjörð Bjarmason
2017-03-21 19:26   ` Junio C Hamano
2017-03-21 19:43   ` Jeff King

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq1stszxn4.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=conrad.irwin@gmail.com \
    --cc=git@drmicha.warpmail.net \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=rhansen@bbn.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=sitaramc@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox