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

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

[Ævar Arnfjörð Bjarmason]

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

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 change was independently authored to scratch a longtime itch, but
when I was about to submit it I discovered that a similar patch had
been submitted unsuccessfully before by Conrad Irwin in August 2011 as
"rev-parse: Allow @{U} as a synonym for @{u}"[1].

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, but the current code does not conflate the parsing of
@{upstream}, @{push}, ^{tree} etc. with any other @{STRING} or ^{TYPE}
we might add in the future and want to keep case-sensitive.

The new starts_with_case() function is a copy of the existing adjacent
starts_with(), just with a tolower() in the "else if".

The tests for this patch are more exhaustive than in the 2011
submission. The starting point for them was to first change the code
to only support upper-case versions of the existing words, seeing what
broke, and amending the breaking tests to check upper case & mixed
case as appropriate, and where not redundant to other similar tests.

1. <1313287071-7851-1-git-send-email-conrad.irwin@gmail.com>
2. <7vhb5fd4zy.fsf@alter.siamese.dyndns.org>

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/revisions.txt   | 11 ++++++++++-
 git-compat-util.h             |  1 +
 sha1_name.c                   | 12 ++++++------
 strbuf.c                      |  9 +++++++++
 t/t1450-fsck.sh               |  7 +++++++
 t/t1507-rev-parse-upstream.sh | 15 +++++++++++----
 t/t1511-rev-parse-caret.sh    | 13 +++++++++++++
 t/t1514-rev-parse-push.sh     |  8 ++++++--
 8 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index ba11b9c95e..55bde6ea65 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -96,7 +96,8 @@ some output processing may assume ref names in UTF-8.
   refers to the branch that the branch specified by branchname is set to build on
   top of (configured with `branch.<name>.remote` and
   `branch.<name>.merge`).  A missing branchname defaults to the
-  current one.
+  current one. Both '@\{upstream\}', '@\{u\}' are case-insensitive, so e.g.
+  '@\{UPSTREAM\}', '@\{U\}' or '@\{Upstream\}' also work.
 
 '<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
   The suffix '@\{push}' reports the branch "where we would push to" if
@@ -122,6 +123,9 @@ refs/remotes/myfork/mybranch
 Note in the example that we set up a triangular workflow, where we pull
 from one location and push to another. In a non-triangular workflow,
 '@\{push}' is the same as '@\{upstream}', and there is no need for it.
++
+'@\{push}' is matched case-insensitively, so e.g. '@\{PUSH}' or
+'@\{Push}' also works.
 
 '<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
@@ -158,6 +162,11 @@ it does not have to be dereferenced even once to get to an object.
 +
 'rev{caret}\{tag\}' can be used to ensure that 'rev' identifies an
 existing tag object.
++
+The {caret}{<type>} part is matched case-insensitively. So
+e.g. '{caret}\{commit\}' can be equivalently specified as
+'{caret}\{COMMIT\}', '{caret}\{Commit\}' etc., '{caret}\{tree\}' as
+'{caret}\{TREE\}' and so forth.
 
 '<rev>{caret}{}', e.g. 'v0.99.8{caret}{}'::
   A suffix '{caret}' followed by an empty brace pair
diff --git a/git-compat-util.h b/git-compat-util.h
index e626851fe9..2b3b581a7c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -448,6 +448,7 @@ extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
 extern int starts_with(const char *str, const char *prefix);
+extern int starts_with_case(const char *str, const char *prefix);
 
 /*
  * If the string "str" begins with the string found in "prefix", return 1.
diff --git a/sha1_name.c b/sha1_name.c
index cda9e49b12..8ff215932d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -549,7 +549,7 @@ static inline int at_mark(const char *string, int len,
 	for (i = 0; i < nr; i++) {
 		int suffix_len = strlen(suffix[i]);
 		if (suffix_len <= len
-		    && !memcmp(string, suffix[i], suffix_len))
+		    && !strncasecmp(string, suffix[i], suffix_len))
 			return suffix_len;
 	}
 	return 0;
@@ -821,15 +821,15 @@ static int peel_onion(const char *name, int len, unsigned char *sha1,
 		return -1;
 
 	sp++; /* beginning of type name, or closing brace for empty */
-	if (starts_with(sp, "commit}"))
+	if (starts_with_case(sp, "commit}"))
 		expected_type = OBJ_COMMIT;
-	else if (starts_with(sp, "tag}"))
+	else if (starts_with_case(sp, "tag}"))
 		expected_type = OBJ_TAG;
-	else if (starts_with(sp, "tree}"))
+	else if (starts_with_case(sp, "tree}"))
 		expected_type = OBJ_TREE;
-	else if (starts_with(sp, "blob}"))
+	else if (starts_with_case(sp, "blob}"))
 		expected_type = OBJ_BLOB;
-	else if (starts_with(sp, "object}"))
+	else if (starts_with_case(sp, "object}"))
 		expected_type = OBJ_ANY;
 	else if (sp[0] == '}')
 		expected_type = OBJ_NONE;
diff --git a/strbuf.c b/strbuf.c
index ace58e7367..d6483381c4 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,6 +11,15 @@ int starts_with(const char *str, const char *prefix)
 			return 0;
 }
 
+int starts_with_case(const char *str, const char *prefix)
+{
+	for (; ; str++, prefix++)
+		if (!*prefix)
+			return 1;
+		else if (tolower(*str) != tolower(*prefix))
+			return 0;
+}
+
 /*
  * Used as the default ->buf value, so that people can always assume
  * buf is non NULL and ->buf is NUL terminated even for a freshly
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 33a51c9a67..b6c1989671 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -505,6 +505,13 @@ test_expect_success 'fsck notices missing tagged object' '
 	test_must_fail git -C missing fsck
 '
 
+test_expect_success 'fsck notices missing tagged object with case insensitive {blob}' '
+	create_repo_missing tag^{BLOB} &&
+	test_must_fail git -C missing fsck &&
+	create_repo_missing tag^{BloB} &&
+	test_must_fail git -C missing fsck
+'
+
 test_expect_success 'fsck notices ref pointing to missing commit' '
 	create_repo_missing HEAD &&
 	test_must_fail git -C missing fsck
diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 46ef1f22dc..b23c4e3fab 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -46,11 +46,14 @@ error_message () {
 }
 
 test_expect_success '@{upstream} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{upstream})"
+	test refs/remotes/origin/master = "$(full_name @{upstream})" &&
+	test refs/remotes/origin/master = "$(full_name @{UPSTREAM})" &&
+	test refs/remotes/origin/master = "$(full_name @{UpSTReam})"
 '
 
 test_expect_success '@{u} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{u})"
+	test refs/remotes/origin/master = "$(full_name @{u})" &&
+	test refs/remotes/origin/master = "$(full_name @{U})"
 '
 
 test_expect_success 'my-side@{upstream} resolves to correct full name' '
@@ -60,6 +63,8 @@ test_expect_success 'my-side@{upstream} resolves to correct full name' '
 test_expect_success 'upstream of branch with @ in middle' '
 	full_name fun@ny@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
+	test_cmp expect actual &&
+	full_name fun@ny@{U} >actual &&
 	test_cmp expect actual
 '
 
@@ -96,12 +101,14 @@ test_expect_success 'not-tracking@{u} fails' '
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
 	test_commit 6 &&
 	(cd clone && git fetch) &&
-	test 5 = $(commit_subject my-side@{u}@{1})
+	test 5 = $(commit_subject my-side@{u}@{1}) &&
+	test 5 = $(commit_subject my-side@{U}@{1})
 '
 
 test_expect_success '@{u} without specifying branch fails on a detached HEAD' '
 	git checkout HEAD^0 &&
-	test_must_fail git rev-parse @{u}
+	test_must_fail git rev-parse @{u} &&
+	test_must_fail git rev-parse @{U}
 '
 
 test_expect_success 'checkout -b new my-side@{u} forks from the same' '
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index e0a49a651f..56750f99c6 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -48,6 +48,10 @@ test_expect_success 'ref^{commit}' '
 	git rev-parse ref >expected &&
 	git rev-parse ref^{commit} >actual &&
 	test_cmp expected actual &&
+	git rev-parse ref^{COMMIT} >actual &&
+	test_cmp expected actual &&
+	git rev-parse ref^{CoMMiT} >actual &&
+	test_cmp expected actual &&
 	git rev-parse commit-tag^{commit} >actual &&
 	test_cmp expected actual &&
 	test_must_fail git rev-parse tree-tag^{commit} &&
@@ -58,6 +62,10 @@ test_expect_success 'ref^{tree}' '
 	echo $TREE_SHA1 >expected &&
 	git rev-parse ref^{tree} >actual &&
 	test_cmp expected actual &&
+	git rev-parse ref^{TREE} >actual &&
+	test_cmp expected actual &&
+	git rev-parse ref^{TrEe} >actual &&
+	test_cmp expected actual &&
 	git rev-parse commit-tag^{tree} >actual &&
 	test_cmp expected actual &&
 	git rev-parse tree-tag^{tree} >actual &&
@@ -67,8 +75,13 @@ test_expect_success 'ref^{tree}' '
 
 test_expect_success 'ref^{tag}' '
 	test_must_fail git rev-parse HEAD^{tag} &&
+	test_must_fail git rev-parse HEAD^{TAG} &&
 	git rev-parse commit-tag >expected &&
 	git rev-parse commit-tag^{tag} >actual &&
+	test_cmp expected actual &&
+	git rev-parse commit-tag^{TAG} >actual &&
+	test_cmp expected actual &&
+	git rev-parse commit-tag^{Tag} >actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 623a32aa6e..788cc91e45 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -24,12 +24,16 @@ test_expect_success 'setup' '
 
 test_expect_success '@{push} with default=nothing' '
 	test_config push.default nothing &&
-	test_must_fail git rev-parse master@{push}
+	test_must_fail git rev-parse master@{push} &&
+	test_must_fail git rev-parse master@{PUSH} &&
+	test_must_fail git rev-parse master@{PuSH}
 '
 
 test_expect_success '@{push} with default=simple' '
 	test_config push.default simple &&
-	resolve master@{push} refs/remotes/origin/master
+	resolve master@{push} refs/remotes/origin/master &&
+	resolve master@{PUSH} refs/remotes/origin/master &&
+	resolve master@{pUSh} refs/remotes/origin/master
 '
 
 test_expect_success 'triangular @{push} fails with default=simple' '
-- 
2.11.0

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

[Duy Nguyen]

On Sun, Mar 19, 2017 at 5:34 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index ba11b9c95e..55bde6ea65 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -96,7 +96,8 @@ some output processing may assume ref names in UTF-8.
>    refers to the branch that the branch specified by branchname is set to build on
>    top of (configured with `branch.<name>.remote` and
>    `branch.<name>.merge`).  A missing branchname defaults to the
> -  current one.
> +  current one. Both '@\{upstream\}', '@\{u\}' are case-insensitive, so e.g.
> +  '@\{UPSTREAM\}', '@\{U\}' or '@\{Upstream\}' also work.

Since this change makes @{everything} case-insensitive and there's no
new one on the horizon, can we just say "everything in @{..} is
case-insensitive unless otherwise specified" and not updating every
@{case}? It sets a new common rule for these @{}, which I think is
good (easier to remember as a user).
-- 
Duy

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

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 19, 2017 at 10:19 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Mar 19, 2017 at 5:34 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
>> index ba11b9c95e..55bde6ea65 100644
>> --- a/Documentation/revisions.txt
>> +++ b/Documentation/revisions.txt
>> @@ -96,7 +96,8 @@ some output processing may assume ref names in UTF-8.
>>    refers to the branch that the branch specified by branchname is set to build on
>>    top of (configured with `branch.<name>.remote` and
>>    `branch.<name>.merge`).  A missing branchname defaults to the
>> -  current one.
>> +  current one. Both '@\{upstream\}', '@\{u\}' are case-insensitive, so e.g.
>> +  '@\{UPSTREAM\}', '@\{U\}' or '@\{Upstream\}' also work.
>
> Since this change makes @{everything} case-insensitive and there's no
> new one on the horizon, can we just say "everything in @{..} is
> case-insensitive unless otherwise specified" and not updating every
> @{case}? It sets a new common rule for these @{}, which I think is
> good (easier to remember as a user).

I have a slight bias to keeping it the way it is, just because of the
distracted user in a hurry use-case that's looking up only @{u} and
not reading the manpage all the way through.

But yeah, I could do that, i.e. have some blurb at the top noting that
all of the magical syntax is case insensitive unless otherwise noted.
AFAICT the only thing that's case sensitive now is the ^{/regex} form.

Also an advantage of that would be that currently we don't note that
e.g. sha1s like dae86e aren't case sensitive and can be written like
DAE86E, and the @{<date>} syntax doesn't note that it's case
insensitive. A blurb at the top would cover that.

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

[Junio C Hamano]

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

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

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

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.

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.  There is no sensible
"consistency" justification possible.

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.

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.

Thanks.

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

[Ævar Arnfjörð Bjarmason]

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

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

[Junio C Hamano]

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

[PATCH v2 0/3] rev-parse case insensitivity & @{p} synonym

[Ævar Arnfjörð Bjarmason]

This v2 addresses the feedback on my "rev-parse: match @{u}, @{push}
and ^{<type>} case-insensitively" patch. I've split this into a 3
patch series:

Ævar Arnfjörð Bjarmason (3):
  rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

Reworded the documentation as Junio suggested in
<xmqq1stszxn4.fsf@gitster.mtv.corp.google.com>.

  rev-parse: add @{p} as a synonym for @{push}

While I'm at it why don't we have a shorthand for @{push} like
@{upstream}? Add it as @{p}.

  rev-parse: match ^{<type>} case-insensitively

Junio didn't want ^{<type>} case-insensitive "for now", so I split it
out of the first patch.

I'm not overly excited about it, neither is Junio, so this'll probably
be dropped, but I wanted to submit it as a standalone patch in case
anyone wanted to pick this up in the future.

 Documentation/revisions.txt   | 15 ++++++++++++---
 git-compat-util.h             |  1 +
 sha1_name.c                   | 14 +++++++-------
 strbuf.c                      |  9 +++++++++
 t/t1450-fsck.sh               |  7 +++++++
 t/t1507-rev-parse-upstream.sh | 15 +++++++++++----
 t/t1511-rev-parse-caret.sh    | 13 +++++++++++++
 t/t1514-rev-parse-push.sh     | 10 ++++++++--
 8 files changed, 68 insertions(+), 16 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

[Ævar Arnfjörð Bjarmason]

Change the revision parsing logic to match @{upstream}, @{u} & @{push}
case-insensitively.

Before this change supplying anything except the lower-case forms
emits an "unknown revision or path not in the working tree"
error. This change makes upper-case & mixed-case versions equivalent
to the lower-case versions.

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.

These suffixes now join various other suffixes & special syntax
documented in gitrevisions(7) that matches case-insensitively. A table
showing the status of the various forms documented there before &
after this patch is shown below. The key for the table is:

 - CI  = Case Insensitive
 - CIP = Case Insensitive Possible (without ambiguities)
 - AG  = Accepts Garbage (.e.g. @{./.4.minutes./.})

Before this change:

    |----------------+-----+------+-----|
    | 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 it:

    |----------------+-----+------+-----|
    | 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>}      | N   | Y    | N   |
    | ^{/regex}      | N   | N    | N   |
    |----------------+-----+------+-----|

The ^{<type>} suffix could be changed to be case-insensitive as well
without introducing any ambiguities. That was included in an earlier
version of this patch, but Junio objected to its inclusion in [2].

This change was independently authored to scratch a longtime itch, but
when I was about to submit it I discovered that a similar patch had
been submitted unsuccessfully before by Conrad Irwin in August 2011 as
"rev-parse: Allow @{U} as a synonym for @{u}"[1].

The tests for this patch are more exhaustive than in the 2011
submission. The starting point for them was to first change the code
to only support upper-case versions of the existing words, seeing what
broke, and amending the breaking tests to check upper case & mixed
case as appropriate, and where not redundant to other similar
tests. The implementation itself is equivalent.

1. <1313287071-7851-1-git-send-email-conrad.irwin@gmail.com>
2. <xmqq1stszxn4.fsf@gitster.mtv.corp.google.com>

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/revisions.txt   |  6 +++++-
 sha1_name.c                   |  2 +-
 t/t1507-rev-parse-upstream.sh | 15 +++++++++++----
 t/t1514-rev-parse-push.sh     |  8 ++++++--
 4 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index ba11b9c95e..09e0d51b9e 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -96,7 +96,8 @@ some output processing may assume ref names in UTF-8.
   refers to the branch that the branch specified by branchname is set to build on
   top of (configured with `branch.<name>.remote` and
   `branch.<name>.merge`).  A missing branchname defaults to the
-  current one.
+  current one. These suffixes are accepted when spelled in uppercase, and
+  they mean the same thing no matter the case.
 
 '<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
   The suffix '@\{push}' reports the branch "where we would push to" if
@@ -122,6 +123,9 @@ refs/remotes/myfork/mybranch
 Note in the example that we set up a triangular workflow, where we pull
 from one location and push to another. In a non-triangular workflow,
 '@\{push}' is the same as '@\{upstream}', and there is no need for it.
++
+This suffix is accepted when spelled in uppercase, and means the same
+thing no matter the case.
 
 '<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
diff --git a/sha1_name.c b/sha1_name.c
index cda9e49b12..d9d1b2fce8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -549,7 +549,7 @@ static inline int at_mark(const char *string, int len,
 	for (i = 0; i < nr; i++) {
 		int suffix_len = strlen(suffix[i]);
 		if (suffix_len <= len
-		    && !memcmp(string, suffix[i], suffix_len))
+		    && !strncasecmp(string, suffix[i], suffix_len))
 			return suffix_len;
 	}
 	return 0;
diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 46ef1f22dc..b23c4e3fab 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -46,11 +46,14 @@ error_message () {
 }
 
 test_expect_success '@{upstream} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{upstream})"
+	test refs/remotes/origin/master = "$(full_name @{upstream})" &&
+	test refs/remotes/origin/master = "$(full_name @{UPSTREAM})" &&
+	test refs/remotes/origin/master = "$(full_name @{UpSTReam})"
 '
 
 test_expect_success '@{u} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{u})"
+	test refs/remotes/origin/master = "$(full_name @{u})" &&
+	test refs/remotes/origin/master = "$(full_name @{U})"
 '
 
 test_expect_success 'my-side@{upstream} resolves to correct full name' '
@@ -60,6 +63,8 @@ test_expect_success 'my-side@{upstream} resolves to correct full name' '
 test_expect_success 'upstream of branch with @ in middle' '
 	full_name fun@ny@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
+	test_cmp expect actual &&
+	full_name fun@ny@{U} >actual &&
 	test_cmp expect actual
 '
 
@@ -96,12 +101,14 @@ test_expect_success 'not-tracking@{u} fails' '
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
 	test_commit 6 &&
 	(cd clone && git fetch) &&
-	test 5 = $(commit_subject my-side@{u}@{1})
+	test 5 = $(commit_subject my-side@{u}@{1}) &&
+	test 5 = $(commit_subject my-side@{U}@{1})
 '
 
 test_expect_success '@{u} without specifying branch fails on a detached HEAD' '
 	git checkout HEAD^0 &&
-	test_must_fail git rev-parse @{u}
+	test_must_fail git rev-parse @{u} &&
+	test_must_fail git rev-parse @{U}
 '
 
 test_expect_success 'checkout -b new my-side@{u} forks from the same' '
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 623a32aa6e..788cc91e45 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -24,12 +24,16 @@ test_expect_success 'setup' '
 
 test_expect_success '@{push} with default=nothing' '
 	test_config push.default nothing &&
-	test_must_fail git rev-parse master@{push}
+	test_must_fail git rev-parse master@{push} &&
+	test_must_fail git rev-parse master@{PUSH} &&
+	test_must_fail git rev-parse master@{PuSH}
 '
 
 test_expect_success '@{push} with default=simple' '
 	test_config push.default simple &&
-	resolve master@{push} refs/remotes/origin/master
+	resolve master@{push} refs/remotes/origin/master &&
+	resolve master@{PUSH} refs/remotes/origin/master &&
+	resolve master@{pUSh} refs/remotes/origin/master
 '
 
 test_expect_success 'triangular @{push} fails with default=simple' '
-- 
2.11.0

Re: [PATCH v2 1/3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

[Junio C Hamano]

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

> The ^{<type>} suffix could be changed to be case-insensitive as well
> without introducing any ambiguities. That was included in an earlier
> version of this patch, but Junio objected to its inclusion in [2].

We try not to be personal in our log message.  It's not like my
objection weighs heavier than objections from others.  The same
number of bytes in the log message can better to spent to allow
readers of "git log" independently to judge the rationle without
referring to external material.

Perhaps replace this entire paragraph, including the URL in [2],
with something like

    The ^{type} suffix is not made case-insensitive, because other
    places that take <type> like "cat-file -t <type>" do want them
    case sensitively (after all we never declared that type names
    are case insensitive). Allowing case-insensitive typename only
    with this syntax will make the resulting Git as a whole
    inconsistent.

Other than that, the change to the code and the new tests all makes
sense to me.

Thanks.

[PATCH v3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

[Ævar Arnfjörð Bjarmason]

Change the revision parsing logic to match @{upstream}, @{u} & @{push}
case-insensitively.

Before this change supplying anything except the lower-case forms
emits an "unknown revision or path not in the working tree"
error. This change makes upper-case & mixed-case versions equivalent
to the lower-case versions.

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.

These suffixes now join various other suffixes & special syntax
documented in gitrevisions(7) that matches case-insensitively. A table
showing the status of the various forms documented there before &
after this patch is shown below. The key for the table is:

 - CI  = Case Insensitive
 - CIP = Case Insensitive Possible (without ambiguities)
 - AG  = Accepts Garbage (.e.g. @{./.4.minutes./.})

Before this change:

    |----------------+-----+------+-----|
    | 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 it:

    |----------------+-----+------+-----|
    | 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>}      | N   | Y    | N   |
    | ^{/regex}      | N   | N    | N   |
    |----------------+-----+------+-----|

The ^{<type>} suffix is not made case-insensitive, because other
places that take <type> like "cat-file -t <type>" do want them case
sensitively (after all we never declared that type names are case
insensitive). Allowing case-insensitive typename only with this syntax
will make the resulting Git as a whole inconsistent.

This change was independently authored to scratch a longtime itch, but
when I was about to submit it I discovered that a similar patch had
been submitted unsuccessfully before by Conrad Irwin in August 2011 as
"rev-parse: Allow @{U} as a synonym for
@{u}" (<1313287071-7851-1-git-send-email-conrad.irwin@gmail.com>).

The tests for this patch are more exhaustive than in the 2011
submission. The starting point for them was to first change the code
to only support upper-case versions of the existing words, seeing what
broke, and amending the breaking tests to check upper case & mixed
case as appropriate, and where not redundant to other similar
tests. The implementation itself is equivalent.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, Mar 27, 2017 at 2:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The ^{<type>} suffix could be changed to be case-insensitive as well
>> without introducing any ambiguities. That was included in an earlier
>> version of this patch, but Junio objected to its inclusion in [2].
>
> We try not to be personal in our log message.  It's not like my
> objection weighs heavier than objections from others.  The same
> number of bytes in the log message can better to spent to allow
> readers of "git log" independently to judge the rationle without
> referring to external material.

FWIW I just cited it because you went into a lot more detail in your
message, and thought I'd link to it in lieu of trying to paraphrase it
at even greater length.

> Perhaps replace this entire paragraph, including the URL in [2],
> with something like
>
>     The ^{type} suffix is not made case-insensitive, because other
>     places that take <type> like "cat-file -t <type>" do want them
>     case sensitively (after all we never declared that type names
>     are case insensitive). Allowing case-insensitive typename only
>     with this syntax will make the resulting Git as a whole
>     inconsistent.
>
> Other than that, the change to the code and the new tests all makes
> sense to me.

Makes sense, replaced that note with that summary. Here's hopefully a
final v3 with that change. I've omitted the other two patches as noted
in the discussion about those two, I don't think it makes sense to
include them.

 Documentation/revisions.txt   |  6 +++++-
 sha1_name.c                   |  2 +-
 t/t1507-rev-parse-upstream.sh | 15 +++++++++++----
 t/t1514-rev-parse-push.sh     |  8 ++++++--
 4 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index ba11b9c95e..09e0d51b9e 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -96,7 +96,8 @@ some output processing may assume ref names in UTF-8.
   refers to the branch that the branch specified by branchname is set to build on
   top of (configured with `branch.<name>.remote` and
   `branch.<name>.merge`).  A missing branchname defaults to the
-  current one.
+  current one. These suffixes are accepted when spelled in uppercase, and
+  they mean the same thing no matter the case.
 
 '<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
   The suffix '@\{push}' reports the branch "where we would push to" if
@@ -122,6 +123,9 @@ refs/remotes/myfork/mybranch
 Note in the example that we set up a triangular workflow, where we pull
 from one location and push to another. In a non-triangular workflow,
 '@\{push}' is the same as '@\{upstream}', and there is no need for it.
++
+This suffix is accepted when spelled in uppercase, and means the same
+thing no matter the case.
 
 '<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
diff --git a/sha1_name.c b/sha1_name.c
index cda9e49b12..d9d1b2fce8 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -549,7 +549,7 @@ static inline int at_mark(const char *string, int len,
 	for (i = 0; i < nr; i++) {
 		int suffix_len = strlen(suffix[i]);
 		if (suffix_len <= len
-		    && !memcmp(string, suffix[i], suffix_len))
+		    && !strncasecmp(string, suffix[i], suffix_len))
 			return suffix_len;
 	}
 	return 0;
diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 46ef1f22dc..b23c4e3fab 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -46,11 +46,14 @@ error_message () {
 }
 
 test_expect_success '@{upstream} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{upstream})"
+	test refs/remotes/origin/master = "$(full_name @{upstream})" &&
+	test refs/remotes/origin/master = "$(full_name @{UPSTREAM})" &&
+	test refs/remotes/origin/master = "$(full_name @{UpSTReam})"
 '
 
 test_expect_success '@{u} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{u})"
+	test refs/remotes/origin/master = "$(full_name @{u})" &&
+	test refs/remotes/origin/master = "$(full_name @{U})"
 '
 
 test_expect_success 'my-side@{upstream} resolves to correct full name' '
@@ -60,6 +63,8 @@ test_expect_success 'my-side@{upstream} resolves to correct full name' '
 test_expect_success 'upstream of branch with @ in middle' '
 	full_name fun@ny@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
+	test_cmp expect actual &&
+	full_name fun@ny@{U} >actual &&
 	test_cmp expect actual
 '
 
@@ -96,12 +101,14 @@ test_expect_success 'not-tracking@{u} fails' '
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
 	test_commit 6 &&
 	(cd clone && git fetch) &&
-	test 5 = $(commit_subject my-side@{u}@{1})
+	test 5 = $(commit_subject my-side@{u}@{1}) &&
+	test 5 = $(commit_subject my-side@{U}@{1})
 '
 
 test_expect_success '@{u} without specifying branch fails on a detached HEAD' '
 	git checkout HEAD^0 &&
-	test_must_fail git rev-parse @{u}
+	test_must_fail git rev-parse @{u} &&
+	test_must_fail git rev-parse @{U}
 '
 
 test_expect_success 'checkout -b new my-side@{u} forks from the same' '
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 623a32aa6e..788cc91e45 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -24,12 +24,16 @@ test_expect_success 'setup' '
 
 test_expect_success '@{push} with default=nothing' '
 	test_config push.default nothing &&
-	test_must_fail git rev-parse master@{push}
+	test_must_fail git rev-parse master@{push} &&
+	test_must_fail git rev-parse master@{PUSH} &&
+	test_must_fail git rev-parse master@{PuSH}
 '
 
 test_expect_success '@{push} with default=simple' '
 	test_config push.default simple &&
-	resolve master@{push} refs/remotes/origin/master
+	resolve master@{push} refs/remotes/origin/master &&
+	resolve master@{PUSH} refs/remotes/origin/master &&
+	resolve master@{pUSh} refs/remotes/origin/master
 '
 
 test_expect_success 'triangular @{push} fails with default=simple' '
-- 
2.11.0

Re: [PATCH v3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

[Junio C Hamano]

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

> Before this change:
>
>     |----------------+-----+------+-----|
>     | 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 it:
>
>     |----------------+-----+------+-----|
>     | 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>}      | N   | Y    | N   |
>     | ^{/regex}      | N   | N    | N   |
>     |----------------+-----+------+-----|

As we are not touching ^{<type>} or ^{/regex}, and it is obvious
numbers do not have cases, I'll trim this down to focus only on
things that are relevant while queuing:

    Before this change:

        |----------------+-----+------+-----|
        | What?          | CI? | CIP? | AG? |
        |----------------+-----+------+-----|
        | @{<date>}      | Y   | Y    | Y   |
        | @{upstream}    | N   | Y    | N   |
        | @{push}        | N   | Y    | N   |
        |----------------+-----+------+-----|

    After it:

        |----------------+-----+------+-----|
        | What?          | CI? | CIP? | AG? |
        |----------------+-----+------+-----|
        | @{<date>}      | Y   | Y    | Y   |
        | @{upstream}    | Y   | Y    | N   |
        | @{push}        | Y   | Y    | N   |
        |----------------+-----+------+-----|

should be sufficient to highlight that it was possible to safely
make these two things case insensitive, and we made so.  

For that matter, I do not know the value of AG? field---it only
serves to show that @{<approxidate>} is an odd-man out and cannot be
used as a good example to follow, but I am too lazy to remove it ;-)

> Makes sense, replaced that note with that summary. Here's hopefully a
> final v3 with that change. I've omitted the other two patches as noted
> in the discussion about those two, I don't think it makes sense to
> include them.

Thanks.

> @@ -122,6 +123,9 @@ refs/remotes/myfork/mybranch
>  Note in the example that we set up a triangular workflow, where we pull
>  from one location and push to another. In a non-triangular workflow,
>  '@\{push}' is the same as '@\{upstream}', and there is no need for it.
> ++
> +This suffix is accepted when spelled in uppercase, and means the same
> +thing no matter the case.

As the above text (including the original) does not explicitly say
that lowercase spelling is canonical, the new text is prone to be
misinterpreted that only the uppercase version is accepted.  I'll
do s/is accepted/is also accepted/ while queuing, but please holler
if there are better ways to phrase this.

Thanks.

Re: [PATCH v3] rev-parse: match @{upstream}, @{u} and @{push} case-insensitively

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 27, 2017 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Before this change:
>>
>>     |----------------+-----+------+-----|
>>     | 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 it:
>>
>>     |----------------+-----+------+-----|
>>     | 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>}      | N   | Y    | N   |
>>     | ^{/regex}      | N   | N    | N   |
>>     |----------------+-----+------+-----|
>
> As we are not touching ^{<type>} or ^{/regex}, and it is obvious
> numbers do not have cases, I'll trim this down to focus only on
> things that are relevant while queuing:
>
>     Before this change:
>
>         |----------------+-----+------+-----|
>         | What?          | CI? | CIP? | AG? |
>         |----------------+-----+------+-----|
>         | @{<date>}      | Y   | Y    | Y   |
>         | @{upstream}    | N   | Y    | N   |
>         | @{push}        | N   | Y    | N   |
>         |----------------+-----+------+-----|
>
>     After it:
>
>         |----------------+-----+------+-----|
>         | What?          | CI? | CIP? | AG? |
>         |----------------+-----+------+-----|
>         | @{<date>}      | Y   | Y    | Y   |
>         | @{upstream}    | Y   | Y    | N   |
>         | @{push}        | Y   | Y    | N   |
>         |----------------+-----+------+-----|
>
> should be sufficient to highlight that it was possible to safely
> make these two things case insensitive, and we made so.
>
> For that matter, I do not know the value of AG? field---it only
> serves to show that @{<approxidate>} is an odd-man out and cannot be
> used as a good example to follow, but I am too lazy to remove it ;-)
>
>> Makes sense, replaced that note with that summary. Here's hopefully a
>> final v3 with that change. I've omitted the other two patches as noted
>> in the discussion about those two, I don't think it makes sense to
>> include them.
>
> Thanks.
>
>> @@ -122,6 +123,9 @@ refs/remotes/myfork/mybranch
>>  Note in the example that we set up a triangular workflow, where we pull
>>  from one location and push to another. In a non-triangular workflow,
>>  '@\{push}' is the same as '@\{upstream}', and there is no need for it.
>> ++
>> +This suffix is accepted when spelled in uppercase, and means the same
>> +thing no matter the case.
>
> As the above text (including the original) does not explicitly say
> that lowercase spelling is canonical, the new text is prone to be
> misinterpreted that only the uppercase version is accepted.  I'll
> do s/is accepted/is also accepted/ while queuing, but please holler
> if there are better ways to phrase this.

All of the above sounds good, thanks for fixing it up.

[PATCH v2 2/3] rev-parse: add @{p} as a synonym for @{push}

[Ævar Arnfjörð Bjarmason]

Add @{p} as a shorthand for @{push} for consistency with the @{u}
shorthand for @{upstream}.

This wasn't added when @{push} was introduced in commit
adfe5d0434 ("sha1_name: implement @{push} shorthand", 2015-05-21), but
it can be added without any ambiguity and saves the user some typing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/revisions.txt | 8 ++++----
 sha1_name.c                 | 2 +-
 t/t1514-rev-parse-push.sh   | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 09e0d51b9e..5fe90e411d 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -99,8 +99,8 @@ some output processing may assume ref names in UTF-8.
   current one. These suffixes are accepted when spelled in uppercase, and
   they mean the same thing no matter the case.
 
-'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
-  The suffix '@\{push}' reports the branch "where we would push to" if
+'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{p\}'::
+  The suffix '@\{push}' (short form '@\{push}') reports the branch "where we would push to" if
   `git push` were run while `branchname` was checked out (or the current
   `HEAD` if no branchname is specified). Since our push destination is
   in a remote repository, of course, we report the local tracking branch
@@ -124,8 +124,8 @@ Note in the example that we set up a triangular workflow, where we pull
 from one location and push to another. In a non-triangular workflow,
 '@\{push}' is the same as '@\{upstream}', and there is no need for it.
 +
-This suffix is accepted when spelled in uppercase, and means the same
-thing no matter the case.
+These suffixes are accepted when spelled in uppercase, and they mean
+the same thing no matter the case.
 
 '<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
diff --git a/sha1_name.c b/sha1_name.c
index d9d1b2fce8..2deb9bfdf6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -563,7 +563,7 @@ static inline int upstream_mark(const char *string, int len)
 
 static inline int push_mark(const char *string, int len)
 {
-	const char *suffix[] = { "@{push}" };
+	const char *suffix[] = { "@{push}", "@{p}" };
 	return at_mark(string, len, suffix, ARRAY_SIZE(suffix));
 }
 
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 788cc91e45..db9aaf88f8 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -31,6 +31,8 @@ test_expect_success '@{push} with default=nothing' '
 
 test_expect_success '@{push} with default=simple' '
 	test_config push.default simple &&
+	resolve master@{p} refs/remotes/origin/master &&
+	resolve master@{P} refs/remotes/origin/master &&
 	resolve master@{push} refs/remotes/origin/master &&
 	resolve master@{PUSH} refs/remotes/origin/master &&
 	resolve master@{pUSh} refs/remotes/origin/master
-- 
2.11.0

Re: [PATCH v2 2/3] rev-parse: add @{p} as a synonym for @{push}

[Jeff King]

On Sun, Mar 26, 2017 at 12:16:53PM +0000, Ævar Arnfjörð Bjarmason wrote:

> Add @{p} as a shorthand for @{push} for consistency with the @{u}
> shorthand for @{upstream}.
> 
> This wasn't added when @{push} was introduced in commit
> adfe5d0434 ("sha1_name: implement @{push} shorthand", 2015-05-21), but
> it can be added without any ambiguity and saves the user some typing.

It _can_ be added, but it was intentionally avoided at the time because
there was discussion of adding other p-words, including:

  - @{pull} as a synonym for @{upstream} (and to better match @{push})

  - @{publish}, which was some similar-ish system that was based on
    per-branch config, but the patches were never merged.

It's been a few years with neither of those things happening, so in a
sense it may be safe to add it now. OTOH, if users are not clamoring for
@{p} and it is just being added "because we can", maybe that is not a
good reason.

> -'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
> -  The suffix '@\{push}' reports the branch "where we would push to" if
> +'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{p\}'::
> +  The suffix '@\{push}' (short form '@\{push}') reports the branch "where we would push to" if

Did you mean to say "short form '@\{p}'"?

-Peff

Re: [PATCH v2 2/3] rev-parse: add @{p} as a synonym for @{push}

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 27, 2017 at 4:53 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Mar 26, 2017 at 12:16:53PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Add @{p} as a shorthand for @{push} for consistency with the @{u}
>> shorthand for @{upstream}.
>>
>> This wasn't added when @{push} was introduced in commit
>> adfe5d0434 ("sha1_name: implement @{push} shorthand", 2015-05-21), but
>> it can be added without any ambiguity and saves the user some typing.
>
> It _can_ be added, but it was intentionally avoided at the time because
> there was discussion of adding other p-words, including:
>
>   - @{pull} as a synonym for @{upstream} (and to better match @{push})
>
>   - @{publish}, which was some similar-ish system that was based on
>     per-branch config, but the patches were never merged.
>
> It's been a few years with neither of those things happening, so in a
> sense it may be safe to add it now. OTOH, if users are not clamoring for
> @{p} and it is just being added "because we can", maybe that is not a
> good reason.

Yeah let's just drop this.

>> -'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{push\}'::
>> -  The suffix '@\{push}' reports the branch "where we would push to" if
>> +'<branchname>@\{push\}', e.g. 'master@\{push\}', '@\{p\}'::
>> +  The suffix '@\{push}' (short form '@\{push}') reports the branch "where we would push to" if
>
> Did you mean to say "short form '@\{p}'"?

Yup, my mistake.

[PATCH v2 3/3] rev-parse: match ^{<type>} case-insensitively

[Ævar Arnfjörð Bjarmason]

Change the revision parsing logic to match ^{commit}, ^{tree}, ^{blob}
etc. case-insensitively.

Before this change supplying anything except the lower-case forms
emits an "unknown revision or path not in the working tree"
error. This change makes upper-case & mixed-case versions equivalent
to the lower-case versions.

The rationale for this change is the same as for making @{upstream}
and related suffixes case-insensitive in "rev-parse: match
@{upstream}, @{u} and @{push} case-insensitively", but unlike those
suffixes this change introduces the potential confusion of accepting
TREE or BLOB here, but not as an argument to e.g. "cat-file -t <type>"
or "hash-object -t <type>".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/revisions.txt |  5 +++++
 git-compat-util.h           |  1 +
 sha1_name.c                 | 10 +++++-----
 strbuf.c                    |  9 +++++++++
 t/t1450-fsck.sh             |  7 +++++++
 t/t1511-rev-parse-caret.sh  | 13 +++++++++++++
 6 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 5fe90e411d..136e26c05d 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -162,6 +162,11 @@ it does not have to be dereferenced even once to get to an object.
 +
 'rev{caret}\{tag\}' can be used to ensure that 'rev' identifies an
 existing tag object.
++
+The {caret}{<type>} part is matched case-insensitively. So
+e.g. '{caret}\{commit\}' can be equivalently specified as
+'{caret}\{COMMIT\}', '{caret}\{Commit\}' etc., '{caret}\{tree\}' as
+'{caret}\{TREE\}' and so forth.
 
 '<rev>{caret}{}', e.g. 'v0.99.8{caret}{}'::
   A suffix '{caret}' followed by an empty brace pair
diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..4a03934ef3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -448,6 +448,7 @@ extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
 extern int starts_with(const char *str, const char *prefix);
+extern int starts_with_icase(const char *str, const char *prefix);
 
 /*
  * If the string "str" begins with the string found in "prefix", return 1.
diff --git a/sha1_name.c b/sha1_name.c
index 2deb9bfdf6..107246bd2d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -821,15 +821,15 @@ static int peel_onion(const char *name, int len, unsigned char *sha1,
 		return -1;
 
 	sp++; /* beginning of type name, or closing brace for empty */
-	if (starts_with(sp, "commit}"))
+	if (starts_with_icase(sp, "commit}"))
 		expected_type = OBJ_COMMIT;
-	else if (starts_with(sp, "tag}"))
+	else if (starts_with_icase(sp, "tag}"))
 		expected_type = OBJ_TAG;
-	else if (starts_with(sp, "tree}"))
+	else if (starts_with_icase(sp, "tree}"))
 		expected_type = OBJ_TREE;
-	else if (starts_with(sp, "blob}"))
+	else if (starts_with_icase(sp, "blob}"))
 		expected_type = OBJ_BLOB;
-	else if (starts_with(sp, "object}"))
+	else if (starts_with_icase(sp, "object}"))
 		expected_type = OBJ_ANY;
 	else if (sp[0] == '}')
 		expected_type = OBJ_NONE;
diff --git a/strbuf.c b/strbuf.c
index ace58e7367..7d4a59bca6 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,6 +11,15 @@ int starts_with(const char *str, const char *prefix)
 			return 0;
 }
 
+int starts_with_icase(const char *str, const char *prefix)
+{
+	for (; ; str++, prefix++)
+		if (!*prefix)
+			return 1;
+		else if (tolower(*str) != tolower(*prefix))
+			return 0;
+}
+
 /*
  * Used as the default ->buf value, so that people can always assume
  * buf is non NULL and ->buf is NUL terminated even for a freshly
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 33a51c9a67..b6c1989671 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -505,6 +505,13 @@ test_expect_success 'fsck notices missing tagged object' '
 	test_must_fail git -C missing fsck
 '
 
+test_expect_success 'fsck notices missing tagged object with case insensitive {blob}' '
+	create_repo_missing tag^{BLOB} &&
+	test_must_fail git -C missing fsck &&
+	create_repo_missing tag^{BloB} &&
+	test_must_fail git -C missing fsck
+'
+
 test_expect_success 'fsck notices ref pointing to missing commit' '
 	create_repo_missing HEAD &&
 	test_must_fail git -C missing fsck
diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index e0a49a651f..56750f99c6 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -48,6 +48,10 @@ test_expect_success 'ref^{commit}' '
 	git rev-parse ref >expected &&
 	git rev-parse ref^{commit} >actual &&
 	test_cmp expected actual &&
+	git rev-parse ref^{COMMIT} >actual &&
+	test_cmp expected actual &&
+	git rev-parse ref^{CoMMiT} >actual &&
+	test_cmp expected actual &&
 	git rev-parse commit-tag^{commit} >actual &&
 	test_cmp expected actual &&
 	test_must_fail git rev-parse tree-tag^{commit} &&
@@ -58,6 +62,10 @@ test_expect_success 'ref^{tree}' '
 	echo $TREE_SHA1 >expected &&
 	git rev-parse ref^{tree} >actual &&
 	test_cmp expected actual &&
+	git rev-parse ref^{TREE} >actual &&
+	test_cmp expected actual &&
+	git rev-parse ref^{TrEe} >actual &&
+	test_cmp expected actual &&
 	git rev-parse commit-tag^{tree} >actual &&
 	test_cmp expected actual &&
 	git rev-parse tree-tag^{tree} >actual &&
@@ -67,8 +75,13 @@ test_expect_success 'ref^{tree}' '
 
 test_expect_success 'ref^{tag}' '
 	test_must_fail git rev-parse HEAD^{tag} &&
+	test_must_fail git rev-parse HEAD^{TAG} &&
 	git rev-parse commit-tag >expected &&
 	git rev-parse commit-tag^{tag} >actual &&
+	test_cmp expected actual &&
+	git rev-parse commit-tag^{TAG} >actual &&
+	test_cmp expected actual &&
+	git rev-parse commit-tag^{Tag} >actual &&
 	test_cmp expected actual
 '
 
-- 
2.11.0

Re: [PATCH v2 3/3] rev-parse: match ^{<type>} case-insensitively

[Junio C Hamano]

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

> Change the revision parsing logic to match ^{commit}, ^{tree}, ^{blob}
> etc. case-insensitively.
>
> Before this change supplying anything except the lower-case forms
> emits an "unknown revision or path not in the working tree"
> error. This change makes upper-case & mixed-case versions equivalent
> to the lower-case versions.
>
> The rationale for this change is the same as for making @{upstream}
> and related suffixes case-insensitive in "rev-parse: match
> @{upstream}, @{u} and @{push} case-insensitively", but unlike those
> suffixes this change introduces the potential confusion of accepting
> TREE or BLOB here, but not as an argument to e.g. "cat-file -t <type>"
> or "hash-object -t <type>".

It's not "potential confusion".  This closes the door for us to
introduce "TREE" as a separate object type in the future.

If we agree to make a declaration that all typenames are officially
spelled in lowercase [*1*] and at the UI level we accept typenames
spelled in any case, then we can adopt this change (and then we need
to update "cat-file -t" etc. to match it).

I do not at all mind to see if the list concensus is to make such a
declaration and permanently close the door for typenames that are
not lowercase, and after seeing such a concensus I'd gladly
appreciate this patch, but I do not want to see a change like this
that decides the future of the system, pretending as an innocuous
change, sneaked in without making sure that everybody is aware of
its implications.


[Footnote]

*1* "officially spelled in lowercase" is necessary to avoid
    confusion, because we are not making typenames case insensitive,
    allowing an object whose raw-bytes in its loose object
    representation before deflating begins with "COMMIT" and take it
    as an object of <commit> type.  Instead, such a declaration will
    make such an object an invalid one (as opposed to "object of an
    unknown type", as it currently is).

Re: [PATCH v2 3/3] rev-parse: match ^{<type>} case-insensitively

[Jeff King]

On Sun, Mar 26, 2017 at 05:36:21PM -0700, Junio C Hamano wrote:

> It's not "potential confusion".  This closes the door for us to
> introduce "TREE" as a separate object type in the future.
> 
> If we agree to make a declaration that all typenames are officially
> spelled in lowercase [*1*] and at the UI level we accept typenames
> spelled in any case, then we can adopt this change (and then we need
> to update "cat-file -t" etc. to match it).
> 
> I do not at all mind to see if the list concensus is to make such a
> declaration and permanently close the door for typenames that are
> not lowercase, and after seeing such a concensus I'd gladly
> appreciate this patch, but I do not want to see a change like this
> that decides the future of the system, pretending as an innocuous
> change, sneaked in without making sure that everybody is aware of
> its implications.

FWIW, I cannot see us ever adding TREE (or Tree) as a separate type.
It's too confusing for no gain. We'd call it "tree2" or something more
obvious.

So I don't mind closing that door, but I'm not sure if a partial
conversion (where some commands are case-insensitive but others aren't
yet) might not leave us in a more confusing place.

I dunno. I guess I have never wanted to type "^{Tree}" in the first
place, so I do not personally see the _benefit_. Which makes it easy to
see even small negatives as a net loss.

-Peff

Re: [PATCH v2 3/3] rev-parse: match ^{<type>} case-insensitively

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> FWIW, I cannot see us ever adding TREE (or Tree) as a separate type.
> It's too confusing for no gain. We'd call it "tree2" or something more
> obvious.

In case it was not clear, I didn't mean to say I _want_ to leave
that door open.  Well, I cannot imagine it was unclear, as I said I
do not at all mind declaring that all object names will be lowercase
to allow us freely downcase what we got at the UI level.

> So I don't mind closing that door, but I'm not sure if a partial
> conversion (where some commands are case-insensitive but others aren't
> yet) might not leave us in a more confusing place.

Exactly.

> I dunno. I guess I have never wanted to type "^{Tree}" in the first
> place, so I do not personally see the _benefit_. Which makes it easy to
> see even small negatives as a net loss.

As to the potential _benefit_, I do not see much either myself, but
we already are seeing somebody cared enough to throw us a patch, so
to some people there are clearly perceived benefit.  I do not think
closing the door for typenames that are not lowercase is a negative
change at all.

I just wanted the patch to make it clear that it is making such a
system-wide design decision and casting it in stone.  Which includes
that "cat-file <type>" and "hash-object -t <type>" get the same
case-insensitivity update and probably writing that design decision
down somewhere in the documentation, perhaps in the glossary where
we talk about the "object type".

Re: [PATCH v2 3/3] rev-parse: match ^{<type>} case-insensitively

[Jeff King]

On Sun, Mar 26, 2017 at 10:39:18PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > FWIW, I cannot see us ever adding TREE (or Tree) as a separate type.
> > It's too confusing for no gain. We'd call it "tree2" or something more
> > obvious.
> 
> In case it was not clear, I didn't mean to say I _want_ to leave
> that door open.  Well, I cannot imagine it was unclear, as I said I
> do not at all mind declaring that all object names will be lowercase
> to allow us freely downcase what we got at the UI level.

No, I understood that. You just mentioned "list consensus" so I was
trying to give my two cents. ;)

> > I dunno. I guess I have never wanted to type "^{Tree}" in the first
> > place, so I do not personally see the _benefit_. Which makes it easy to
> > see even small negatives as a net loss.
> 
> As to the potential _benefit_, I do not see much either myself, but
> we already are seeing somebody cared enough to throw us a patch, so
> to some people there are clearly perceived benefit.  I do not think
> closing the door for typenames that are not lowercase is a negative
> change at all.

By negative, I just meant potential confusion when we are half-way there
(e.g., "foo^{TREE}" works but "git cat-file TREE foo" does not).

> I just wanted the patch to make it clear that it is making such a
> system-wide design decision and casting it in stone.  Which includes
> that "cat-file <type>" and "hash-object -t <type>" get the same
> case-insensitivity update and probably writing that design decision
> down somewhere in the documentation, perhaps in the glossary where
> we talk about the "object type".

Yes, I agree that that is the right path forward.

-Peff

Re: [PATCH v2 3/3] rev-parse: match ^{<type>} case-insensitively

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 27, 2017 at 4:58 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Mar 26, 2017 at 05:36:21PM -0700, Junio C Hamano wrote:
>
>> It's not "potential confusion".  This closes the door for us to
>> introduce "TREE" as a separate object type in the future.
>>
>> If we agree to make a declaration that all typenames are officially
>> spelled in lowercase [*1*] and at the UI level we accept typenames
>> spelled in any case, then we can adopt this change (and then we need
>> to update "cat-file -t" etc. to match it).
>>
>> I do not at all mind to see if the list concensus is to make such a
>> declaration and permanently close the door for typenames that are
>> not lowercase, and after seeing such a concensus I'd gladly
>> appreciate this patch, but I do not want to see a change like this
>> that decides the future of the system, pretending as an innocuous
>> change, sneaked in without making sure that everybody is aware of
>> its implications.
>
> FWIW, I cannot see us ever adding TREE (or Tree) as a separate type.
> It's too confusing for no gain. We'd call it "tree2" or something more
> obvious.
>
> So I don't mind closing that door, but I'm not sure if a partial
> conversion (where some commands are case-insensitive but others aren't
> yet) might not leave us in a more confusing place.
>
> I dunno. I guess I have never wanted to type "^{Tree}" in the first
> place, so I do not personally see the _benefit_. Which makes it easy to
> see even small negatives as a net loss.

I don't either, I think this patch should just be dropped.

As noted in the cover letter[1] I carved this off from the rest of the
series just in case anyone wanted this, either now or in the future.

I originally implemented this for consistency with @{U} because it's
another thing that can be made case-insensitive unambiguously as far
as the rev-parse syntax is concerned, but as you/Junio note here the
tree/blob/object etc. names exist in a lot more places, so just making
this particular thing case insensitive wouldn't make sense, and has
little benefit.

1. <20170326121654.22035-1-avarab@gmail.com>

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

[Ævar Arnfjörð Bjarmason]

[Dropping Richard Hansen from CC, his E-Mail bounces, changed jobs probably...]

On Sat, Mar 18, 2017 at 11:34 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:

> The new starts_with_case() function is a copy of the existing adjacent
> starts_with(), just with a tolower() in the "else if".
> [...]
> +int starts_with_case(const char *str, const char *prefix)
> +{
> +       for (; ; str++, prefix++)
> +               if (!*prefix)
> +                       return 1;
> +               else if (tolower(*str) != tolower(*prefix))
> +                       return 0;
> +}
> +
>  /*

One thing I'd like feedback on is whether I should be adding this to
strbuf.c. There are >300 uses of starts_with(), but sha1_name.c will
be the only one using this modified starts_with_case() function.
Wouldn't it be better to just add it to sha1_name.c rather than
expanding the strbuf API with something that'll likely be used by
nothing else for a while?

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

[Junio C Hamano]

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

> On Sat, Mar 18, 2017 at 11:34 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>
>> The new starts_with_case() function is a copy of the existing adjacent
>> starts_with(), just with a tolower() in the "else if".
>> [...]
>> +int starts_with_case(const char *str, const char *prefix)
>> +{
>> +       for (; ; str++, prefix++)
>> +               if (!*prefix)
>> +                       return 1;
>> +               else if (tolower(*str) != tolower(*prefix))
>> +                       return 0;
>> +}
>> +
>>  /*
>
> One thing I'd like feedback on is whether I should be adding this to
> strbuf.c. There are >300 uses of starts_with(), but sha1_name.c will
> be the only one using this modified starts_with_case() function.
> Wouldn't it be better to just add it to sha1_name.c rather than
> expanding the strbuf API with something that'll likely be used by
> nothing else for a while?

Yeah, static inside sha1_name.c is OK; people with newer needs can
move it later if necessary.

I'd have called starts_with_icase(), though.

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

[Jeff King]

On Tue, Mar 21, 2017 at 08:19:34PM +0100, Ævar Arnfjörð Bjarmason wrote:

> On Sat, Mar 18, 2017 at 11:34 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> 
> > The new starts_with_case() function is a copy of the existing adjacent
> > starts_with(), just with a tolower() in the "else if".
> > [...]
> > +int starts_with_case(const char *str, const char *prefix)
> > +{
> > +       for (; ; str++, prefix++)
> > +               if (!*prefix)
> > +                       return 1;
> > +               else if (tolower(*str) != tolower(*prefix))
> > +                       return 0;
> > +}
> > +
> >  /*
> 
> One thing I'd like feedback on is whether I should be adding this to
> strbuf.c. There are >300 uses of starts_with(), but sha1_name.c will
> be the only one using this modified starts_with_case() function.
> Wouldn't it be better to just add it to sha1_name.c rather than
> expanding the strbuf API with something that'll likely be used by
> nothing else for a while?

I was thinking that I had written this same function before (in a patch
that didn't end up merged), but it was actually the related
skip_prefix_icase(). I'm fine with it either next to starts_with() or as
a static next to its callers.

-Peff