[PATCH 1/2] refs: move is_pseudoref_syntax() earlier in the file

[PATCH 1/2] refs: move is_pseudoref_syntax() earlier in the file

[Ævar Arnfjörð Bjarmason]

This move is needed by a subsequent change to the expand_ref()
function, which will make use of these functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/refs.c b/refs.c
index 392f0bbf68b..3ec5dcba0be 100644
--- a/refs.c
+++ b/refs.c
@@ -630,6 +630,25 @@ int repo_dwim_ref(struct repository *r, const char *str, int len,
 	return refs_found;
 }
 
+static int is_pseudoref_syntax(const char *refname)
+{
+	const char *c;
+
+	for (c = refname; *c; c++) {
+		if (!isupper(*c) && *c != '-' && *c != '_')
+			return 0;
+	}
+
+	return 1;
+}
+
+static int is_main_pseudoref_syntax(const char *refname)
+{
+	return skip_prefix(refname, "main-worktree/", &refname) &&
+		*refname &&
+		is_pseudoref_syntax(refname);
+}
+
 int expand_ref(struct repository *repo, const char *str, int len,
 	       struct object_id *oid, char **ref)
 {
@@ -716,25 +735,6 @@ static int is_per_worktree_ref(const char *refname)
 	       starts_with(refname, "refs/rewritten/");
 }
 
-static int is_pseudoref_syntax(const char *refname)
-{
-	const char *c;
-
-	for (c = refname; *c; c++) {
-		if (!isupper(*c) && *c != '-' && *c != '_')
-			return 0;
-	}
-
-	return 1;
-}
-
-static int is_main_pseudoref_syntax(const char *refname)
-{
-	return skip_prefix(refname, "main-worktree/", &refname) &&
-		*refname &&
-		is_pseudoref_syntax(refname);
-}
-
 static int is_other_pseudoref_syntax(const char *refname)
 {
 	if (!skip_prefix(refname, "worktrees/", &refname))
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Ævar Arnfjörð Bjarmason]

The refs parsing machinery will first try to parse arbitrary
.git/<name> for a given <name>, before moving onto refs/<name>,
refs/tags/<name> etc. See "ref_rev_parse_rules" in refs.c, but
e.g. "for-each-ref" and other things that list references ignore these
ancient-style refs.

Thus if you end up in a repository that contains e.g. .git/master the
likes of "checkout" can emit seemingly nonsensical error
messages. E.g. I happened to have a .git/master with a non-commit
SHA-1:

    $ git checkout master
    fatal: Cannot switch branch to a non-commit 'master'

Running "for-each-ref" yields only commits that could match "master",
until I realized I'd ended up with a .git/master file. Before this
we'd ignore it under a general rule that tries to ignore .git/HEAD,
.git/MERGE_HEAD and other non-pseudoref looking refs at the top-level.

Let's help the user in this case by doing a very loose check for
whether the ref name looks like a pseudoref such as "HEAD" (i.e. only
has upper case, dashes, underbars), and if not issue a warning:

    $ git rev-parse master
    warning: matched ref .git/master doesn't look like a pseudoref
    c87c83a2e9eb6d309913a0f59389f808024a58f9

I think it's conservative enough to just turn this on by default, but
place it under a configurable option similar to the existing
core.warnAmbiguousRefs. Running the entire test suite with "die"
instead of "warning" passes with this approach.

Our own test suite makes use of a few refs in .git/ that aren't
produced by git itself, e.g. "FOO", "TESTSYMREFTWO" etc, external
tools probably rely on this as well, so I don't think it's viable to
e.g. have a whitelist of "HEAD", "MERGE_HEAD" etc. As an aside that
list is quite large, I counted 12 names used in the C code before I
abandoned that approach.

This approach of checking the case of e.g. "master" is not an issue on
case-insensitive filesystems, since we're not checking against the
fs's version of the name, but what the user provided to git on the
command-line.

We are going to match "git rev-parse master" to e.g. .git/MASTER on
those systems, but I think that's also a case the user would like to
be warned about. I once helped a user on OSX with an issue where they
couldn't repeat a merge command on Linux, and it turned out they'd
referred to "HEAD" as "head", which we'd happily resolve to .git/HEAD
without warning on that system. Now we'll warn about that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/core.txt | 11 ++++++++++
 cache.h                       |  1 +
 config.c                      |  5 +++++
 environment.c                 |  1 +
 refs.c                        | 20 +++++++++++++++++
 t/t1430-bad-ref-name.sh       | 41 +++++++++++++++++++++++++++++++++++
 6 files changed, 79 insertions(+)

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 160aacad84b..535a7012156 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -355,6 +355,17 @@ core.warnAmbiguousRefs::
 	If true, Git will warn you if the ref name you passed it is ambiguous
 	and might match multiple refs in the repository. True by default.
 
+core.warnNonPseudoRefs::
+	If true, Git will warn you if the `<ref>` you passed
+	unexpectedly resolves to a top-level ref stored in
+	`.git/<file>` but doesn't look like a pseudoref such as
+	`HEAD`, `MERGE_HEAD` etc. True by default.
++
+These references are ignored by linkgit:for-each-ref[1], but resolved
+by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be
+confusing to have e.g. an errant `.git/master` being confused with
+`.git/refs/heads/master`.
+
 core.compression::
 	An integer -1..9, indicating a default compression level.
 	-1 is the zlib default. 0 means no compression,
diff --git a/cache.h b/cache.h
index 8d279bc1103..1a0cc5e38a3 100644
--- a/cache.h
+++ b/cache.h
@@ -920,6 +920,7 @@ extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int warn_ambiguous_refs;
+extern int warn_non_pseudo_refs;
 extern int warn_on_object_refname_ambiguity;
 extern char *apply_default_whitespace;
 extern char *apply_default_ignorewhitespace;
diff --git a/config.c b/config.c
index 1137bd73aff..6a589a770f4 100644
--- a/config.c
+++ b/config.c
@@ -1212,6 +1212,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.warnnonpseudorefs")) {
+		warn_non_pseudo_refs = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.abbrev")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/environment.c b/environment.c
index bb518c61cd2..85a84eceaf3 100644
--- a/environment.c
+++ b/environment.c
@@ -29,6 +29,7 @@ int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
+int warn_non_pseudo_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
 int ref_paranoia = -1;
 int repository_format_precious_objects;
diff --git a/refs.c b/refs.c
index 3ec5dcba0be..0beeb3dded8 100644
--- a/refs.c
+++ b/refs.c
@@ -649,12 +649,19 @@ static int is_main_pseudoref_syntax(const char *refname)
 		is_pseudoref_syntax(refname);
 }
 
+static int is_any_pseudoref_syntax(const char *refname)
+{
+	return is_main_pseudoref_syntax(refname) ||
+		is_pseudoref_syntax(refname);
+}
+
 int expand_ref(struct repository *repo, const char *str, int len,
 	       struct object_id *oid, char **ref)
 {
 	const char **p, *r;
 	int refs_found = 0;
 	struct strbuf fullref = STRBUF_INIT;
+	static int warned_on_non_pseudo_ref;
 
 	*ref = NULL;
 	for (p = ref_rev_parse_rules; *p; p++) {
@@ -669,6 +676,19 @@ int expand_ref(struct repository *repo, const char *str, int len,
 					    fullref.buf, RESOLVE_REF_READING,
 					    this_result, &flag);
 		if (r) {
+			if (warn_non_pseudo_refs &&
+			    !starts_with(fullref.buf, "refs/") &&
+			    !starts_with(r, "refs/") &&
+			    !strchr(r, '/') &&
+			    !is_any_pseudoref_syntax(r) &&
+			    !warned_on_non_pseudo_ref++) {
+				/*
+				 * TRANSLATORS: The 1st argument is
+				 * e.g. "master", and the 2nd can be
+				 * e.g. "master~10".
+				 */
+				warning(_("matched ref name .git/%s doesn't look like a pseudoref"), r);
+			}
 			if (!refs_found++)
 				*ref = xstrdup(r);
 			if (!warn_ambiguous_refs)
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index c7878a60edf..2c8c2c15c40 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '
 	git show-ref refs/heads/dash
 '
 
+test_expect_success 'warn on non-pseudoref syntax refs in .git/' '
+	test_when_finished "
+		rm -f .git/mybranch &&
+		rm -rf .git/a-dir &&
+		rm -rf .git/MY-BRANCH_NAME &&
+		rm -rf .git/MY-branch_NAME
+	" &&
+
+	# Setup
+	git rev-parse master >expect &&
+
+	# We ignore anything with slashes
+	mkdir .git/a-dir &&
+	cp expect .git/a-dir/mybranch &&
+	git rev-parse a-dir/mybranch >hash 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect hash &&
+
+	# We ignore upper-case
+	cp expect .git/MY-BRANCH_NAME &&
+	git rev-parse a-dir/mybranch >hash 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect hash &&
+
+	# We ignore mixed-case
+	cp expect .git/MY-branch_NAME &&
+	git rev-parse a-dir/mybranch >hash 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect hash &&
+
+	# We do not ignore lower-case
+	cp expect .git/mybranch &&
+	git rev-parse mybranch >hash 2>err &&
+	test_cmp expect hash &&
+	GIT_TEST_GETTEXT_POISON=false grep "like a pseudoref" err &&
+	git -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&
+	test_cmp expect hash &&
+	test_must_be_empty err &&
+	rm .git/mybranch
+'
+
 test_done
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Eric Sunshine]

On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> [...]
> Let's help the user in this case by doing a very loose check for
> whether the ref name looks like a pseudoref such as "HEAD" (i.e. only
> has upper case, dashes, underbars), and if not issue a warning:
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> @@ -355,6 +355,17 @@ core.warnAmbiguousRefs::
> +core.warnNonPseudoRefs::
> +       If true, Git will warn you if the `<ref>` you passed
> +       unexpectedly resolves to a top-level ref stored in
> +       `.git/<file>` but doesn't look like a pseudoref such as
> +       `HEAD`, `MERGE_HEAD` etc. True by default.
> ++
> +These references are ignored by linkgit:for-each-ref[1], but resolved
> +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be
> +confusing to have e.g. an errant `.git/master` being confused with
> +`.git/refs/heads/master`.

Dscho has been submitting patches lately to eradicate the word
"master" from the project source.

> diff --git a/refs.c b/refs.c
> @@ -669,6 +676,19 @@ int expand_ref(struct repository *repo, const char *str, int len,
>                 if (r) {
> +                       if (warn_non_pseudo_refs &&
> +                           !starts_with(fullref.buf, "refs/") &&
> +                           !starts_with(r, "refs/") &&
> +                           !strchr(r, '/') &&
> +                           !is_any_pseudoref_syntax(r) &&
> +                           !warned_on_non_pseudo_ref++) {
> +                               /*
> +                                * TRANSLATORS: The 1st argument is
> +                                * e.g. "master", and the 2nd can be
> +                                * e.g. "master~10".
> +                                */
> +                               warning(_("matched ref name .git/%s doesn't look like a pseudoref"), r);

The TRANSLATORS comment talks about two arguments, but I see only one.

Does the "matched ref name" part add any value? I would find the
warning just as helpful without it:

    .git/blork doesn't look like a pseudoref

> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> @@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '
> +test_expect_success 'warn on non-pseudoref syntax refs in .git/' '
> +       test_when_finished "
> +               rm -f .git/mybranch &&
> +               rm -rf .git/a-dir &&
> +               rm -rf .git/MY-BRANCH_NAME &&
> +               rm -rf .git/MY-branch_NAME
> +       " &&

Nit: These could all be removed with a single `rm -rf`:

    rm -rf .git/mybranch .git/a-dir ...

> +       # We do not ignore lower-case
> +       cp expect .git/mybranch &&
> +       git rev-parse mybranch >hash 2>err &&
> +       test_cmp expect hash &&
> +       GIT_TEST_GETTEXT_POISON=false grep "like a pseudoref" err &&

What is the purpose of assigning GIT_TEST_GETTEXT_POISON here?

> +       git -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&
> +       test_cmp expect hash &&
> +       test_must_be_empty err &&
> +       rm .git/mybranch
> +'

Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 10 2020, Eric Sunshine wrote:

> On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> [...]
>> Let's help the user in this case by doing a very loose check for
>> whether the ref name looks like a pseudoref such as "HEAD" (i.e. only
>> has upper case, dashes, underbars), and if not issue a warning:
>> [...]
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
>> @@ -355,6 +355,17 @@ core.warnAmbiguousRefs::
>> +core.warnNonPseudoRefs::
>> +       If true, Git will warn you if the `<ref>` you passed
>> +       unexpectedly resolves to a top-level ref stored in
>> +       `.git/<file>` but doesn't look like a pseudoref such as
>> +       `HEAD`, `MERGE_HEAD` etc. True by default.
>> ++
>> +These references are ignored by linkgit:for-each-ref[1], but resolved
>> +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be
>> +confusing to have e.g. an errant `.git/master` being confused with
>> +`.git/refs/heads/master`.
>
> Dscho has been submitting patches lately to eradicate the word
> "master" from the project source.

Muscle memory dies hard, will fix it in the reroll.

>> diff --git a/refs.c b/refs.c
>> @@ -669,6 +676,19 @@ int expand_ref(struct repository *repo, const char *str, int len,
>>                 if (r) {
>> +                       if (warn_non_pseudo_refs &&
>> +                           !starts_with(fullref.buf, "refs/") &&
>> +                           !starts_with(r, "refs/") &&
>> +                           !strchr(r, '/') &&
>> +                           !is_any_pseudoref_syntax(r) &&
>> +                           !warned_on_non_pseudo_ref++) {
>> +                               /*
>> +                                * TRANSLATORS: The 1st argument is
>> +                                * e.g. "master", and the 2nd can be
>> +                                * e.g. "master~10".
>> +                                */
>> +                               warning(_("matched ref name .git/%s doesn't look like a pseudoref"), r);
>
> The TRANSLATORS comment talks about two arguments, but I see only one.
>
> Does the "matched ref name" part add any value? I would find the
> warning just as helpful without it:
>
>     .git/blork doesn't look like a pseudoref

Yeah that's much better, the TRANSLATORS comment is incorrect, rebased
out an earlier version where it took two, didn't notice...

>> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
>> @@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '
>> +test_expect_success 'warn on non-pseudoref syntax refs in .git/' '
>> +       test_when_finished "
>> +               rm -f .git/mybranch &&
>> +               rm -rf .git/a-dir &&
>> +               rm -rf .git/MY-BRANCH_NAME &&
>> +               rm -rf .git/MY-branch_NAME
>> +       " &&
>
> Nit: These could all be removed with a single `rm -rf`:
>
>     rm -rf .git/mybranch .git/a-dir ...

Will fix.

>> +       # We do not ignore lower-case
>> +       cp expect .git/mybranch &&
>> +       git rev-parse mybranch >hash 2>err &&
>> +       test_cmp expect hash &&
>> +       GIT_TEST_GETTEXT_POISON=false grep "like a pseudoref" err &&
>
> What is the purpose of assigning GIT_TEST_GETTEXT_POISON here?

Since 6cdccfce1e0 (i18n: make GETTEXT_POISON a runtime option,
2018-11-08) we haven't needed to use the C_LOCALE_OUTPUT prerequisite
for any new code, since we can just turn the poisoning off.

I think we should just slowly refactor things away from that
prerequisite and test_i18ngrep, which were only needed because it used
to be a compile-time switch, but I haven't gotter around to that
refactoring.

In liue of that I think it makes more sense to always run the full test
if possible, no matter what the GIT_TEST_* mode is.

>> +       git -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&
>> +       test_cmp expect hash &&
>> +       test_must_be_empty err &&
>> +       rm .git/mybranch
>> +'

Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Eric Sunshine]

On Thu, Dec 10, 2020 at 3:29 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Dec 10 2020, Eric Sunshine wrote:
> > On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >> +       GIT_TEST_GETTEXT_POISON=false grep "like a pseudoref" err &&
> >
> > What is the purpose of assigning GIT_TEST_GETTEXT_POISON here?
>
> Since 6cdccfce1e0 (i18n: make GETTEXT_POISON a runtime option,
> 2018-11-08) we haven't needed to use the C_LOCALE_OUTPUT prerequisite
> for any new code, since we can just turn the poisoning off.
>
> I think we should just slowly refactor things away from that
> prerequisite and test_i18ngrep, which were only needed because it used
> to be a compile-time switch, but I haven't gotter around to that
> refactoring.
>
> In liue of that I think it makes more sense to always run the full test
> if possible, no matter what the GIT_TEST_* mode is.

I must be missing something. I've looked at 6cdccfce1e0 but I still
don't see how or why `GIT_TEST_GETTEXT_POISON=false` could affect the
simple `grep` invocation being done by this test. I could understand
if GIT_TEST_GETTEXT_POISON was applied to the invocation of a Git
command, but that's not the case here.

(I also notice that 6cdccfce1e0 only checks whether
GIT_TEST_GETTEXT_POISON is empty or not -- and the changes in
6cdccfce1e0 set GIT_TEST_GETTEXT_POISON to an empty value rather than
to "false", so I find myself doubly confused by this application of
GIT_TEST_GETTEXT_POISON to `grep`.)

Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 10 2020, Eric Sunshine wrote:

> On Thu, Dec 10, 2020 at 3:29 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Thu, Dec 10 2020, Eric Sunshine wrote:
>> > On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
>> >> +       GIT_TEST_GETTEXT_POISON=false grep "like a pseudoref" err &&
>> >
>> > What is the purpose of assigning GIT_TEST_GETTEXT_POISON here?
>>
>> Since 6cdccfce1e0 (i18n: make GETTEXT_POISON a runtime option,
>> 2018-11-08) we haven't needed to use the C_LOCALE_OUTPUT prerequisite
>> for any new code, since we can just turn the poisoning off.
>>
>> I think we should just slowly refactor things away from that
>> prerequisite and test_i18ngrep, which were only needed because it used
>> to be a compile-time switch, but I haven't gotter around to that
>> refactoring.
>>
>> In liue of that I think it makes more sense to always run the full test
>> if possible, no matter what the GIT_TEST_* mode is.
>
> I must be missing something. I've looked at 6cdccfce1e0 but I still
> don't see how or why `GIT_TEST_GETTEXT_POISON=false` could affect the
> simple `grep` invocation being done by this test. I could understand
> if GIT_TEST_GETTEXT_POISON was applied to the invocation of a Git
> command, but that's not the case here.
>
> (I also notice that 6cdccfce1e0 only checks whether
> GIT_TEST_GETTEXT_POISON is empty or not -- and the changes in
> 6cdccfce1e0 set GIT_TEST_GETTEXT_POISON to an empty value rather than
> to "false", so I find myself doubly confused by this application of
> GIT_TEST_GETTEXT_POISON to `grep`.)

You're not missing something, but it seems I need to step away from the
computer for the day. I managed to write this & reply to your E-Mail
without noticing I'd put the GIT_TEST[...] env variable in front of the
wrong command. It should indeed be in front of the rev-parse above.

[PATCH v2 0/2] warn on non-pseudoref looking .git/<file> refs

[Ævar Arnfjörð Bjarmason]

Now with a cover letter to show the range-diff. I addressed all of
Eric's comments, and found some other minor issues myself. The check
for whether we need to warn has also been simplified, we just needed
to check if we have "/" in the resolved "r" variable.

Ævar Arnfjörð Bjarmason (2):
  refs: move is_pseudoref_syntax() earlier in the file
  refs: warn on non-pseudoref looking .git/<file> refs

 Documentation/config/core.txt | 11 ++++++++
 cache.h                       |  1 +
 config.c                      |  5 ++++
 environment.c                 |  1 +
 refs.c                        | 50 ++++++++++++++++++++++-------------
 t/t1430-bad-ref-name.sh       | 41 ++++++++++++++++++++++++++++
 6 files changed, 90 insertions(+), 19 deletions(-)

Range-diff:
-:  ----------- > 1:  e86e55f2828 refs: move is_pseudoref_syntax() earlier in the file
1:  cad73aba664 ! 2:  0573da1d381 refs: warn on non-pseudoref looking .git/<file> refs
    @@ Commit message
     
         The refs parsing machinery will first try to parse arbitrary
         .git/<name> for a given <name>, before moving onto refs/<name>,
    -    refs/tags/<name> etc. See "ref_rev_parse_rules" in refs.c, but
    -    e.g. "for-each-ref" and other things that list references ignore these
    -    ancient-style refs.
    +    refs/tags/<name> etc. See "ref_rev_parse_rules" in refs.c. Things that
    +    list references such as "for-each-ref" ignore these on the assumption
    +    that they're pseudorefs such as "HEAD".
     
         Thus if you end up in a repository that contains e.g. .git/master the
         likes of "checkout" can emit seemingly nonsensical error
    @@ Commit message
         .git/MERGE_HEAD and other non-pseudoref looking refs at the top-level.
     
         Let's help the user in this case by doing a very loose check for
    -    whether the ref name looks like a pseudoref such as "HEAD" (i.e. only
    -    has upper case, dashes, underbars), and if not issue a warning:
    +    whether the ref name looks like a special pseudoref such as
    +    "HEAD" (i.e. only has upper case, dashes, underbars), and if not issue
    +    a warning:
     
             $ git rev-parse master
             warning: matched ref .git/master doesn't look like a pseudoref
    @@ Commit message
         instead of "warning" passes with this approach.
     
         Our own test suite makes use of a few refs in .git/ that aren't
    -    produced by git itself, e.g. "FOO", "TESTSYMREFTWO" etc, external
    +    produced by git itself, e.g. "FOO", "TESTSYMREFTWO" etc. External
         tools probably rely on this as well, so I don't think it's viable to
    -    e.g. have a whitelist of "HEAD", "MERGE_HEAD" etc. As an aside that
    -    list is quite large, I counted 12 names used in the C code before I
    -    abandoned that approach.
    +    e.g. have a whitelist of them. That list is quite large just fr
    +    git.git, I counted 12 names used in the C code before I abandoned that
    +    approach.
     
         This approach of checking the case of e.g. "master" is not an issue on
         case-insensitive filesystems, since we're not checking against the
    @@ Commit message
         without warning on that system. Now we'll warn about that.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/config/core.txt ##
     @@ Documentation/config/core.txt: core.warnAmbiguousRefs::
    @@ Documentation/config/core.txt: core.warnAmbiguousRefs::
     ++
     +These references are ignored by linkgit:for-each-ref[1], but resolved
     +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be
    -+confusing to have e.g. an errant `.git/master` being confused with
    -+`.git/refs/heads/master`.
    ++confusing to have e.g. an errant `.git/mybranch` being confused with
    ++`.git/refs/heads/mybranch`.
     +
      core.compression::
      	An integer -1..9, indicating a default compression level.
    @@ refs.c: int expand_ref(struct repository *repo, const char *str, int len,
      					    this_result, &flag);
      		if (r) {
     +			if (warn_non_pseudo_refs &&
    -+			    !starts_with(fullref.buf, "refs/") &&
    -+			    !starts_with(r, "refs/") &&
     +			    !strchr(r, '/') &&
     +			    !is_any_pseudoref_syntax(r) &&
    -+			    !warned_on_non_pseudo_ref++) {
    -+				/*
    -+				 * TRANSLATORS: The 1st argument is
    -+				 * e.g. "master", and the 2nd can be
    -+				 * e.g. "master~10".
    -+				 */
    -+				warning(_("matched ref name .git/%s doesn't look like a pseudoref"), r);
    -+			}
    ++			    !warned_on_non_pseudo_ref++)
    ++				warning(_(".git/%s doesn't look like a pseudoref"), r);
      			if (!refs_found++)
      				*ref = xstrdup(r);
      			if (!warn_ambiguous_refs)
    @@ t/t1430-bad-ref-name.sh: test_expect_success 'branch -m can rename refs/heads/-d
      
     +test_expect_success 'warn on non-pseudoref syntax refs in .git/' '
     +	test_when_finished "
    -+		rm -f .git/mybranch &&
    -+		rm -rf .git/a-dir &&
    -+		rm -rf .git/MY-BRANCH_NAME &&
    -+		rm -rf .git/MY-branch_NAME
    ++		rm -rf .git/mybranch \
    ++			.git/a-dir \
    ++			.git/MY-BRANCH_NAME \
    ++			.git/MY-branch_NAME
     +	" &&
     +
     +	# Setup
    -+	git rev-parse master >expect &&
    ++	git rev-parse HEAD >expect &&
    ++	mkdir .git/a-dir &&
     +
     +	# We ignore anything with slashes
    -+	mkdir .git/a-dir &&
     +	cp expect .git/a-dir/mybranch &&
     +	git rev-parse a-dir/mybranch >hash 2>err &&
     +	test_must_be_empty err &&
    @@ t/t1430-bad-ref-name.sh: test_expect_success 'branch -m can rename refs/heads/-d
     +
     +	# We do not ignore lower-case
     +	cp expect .git/mybranch &&
    -+	git rev-parse mybranch >hash 2>err &&
    ++	env GIT_TEST_GETTEXT_POISON=false \
    ++		git rev-parse mybranch >hash 2>err &&
     +	test_cmp expect hash &&
    -+	GIT_TEST_GETTEXT_POISON=false grep "like a pseudoref" err &&
    ++	grep "like a pseudoref" err &&
     +	git -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&
     +	test_cmp expect hash &&
    -+	test_must_be_empty err &&
    -+	rm .git/mybranch
    ++	test_must_be_empty err
     +'
     +
      test_done
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 1/2] refs: move is_pseudoref_syntax() earlier in the file

[Ævar Arnfjörð Bjarmason]

This move is needed by a subsequent change to the expand_ref()
function, which will make use of these functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/refs.c b/refs.c
index 392f0bbf68b..3ec5dcba0be 100644
--- a/refs.c
+++ b/refs.c
@@ -630,6 +630,25 @@ int repo_dwim_ref(struct repository *r, const char *str, int len,
 	return refs_found;
 }
 
+static int is_pseudoref_syntax(const char *refname)
+{
+	const char *c;
+
+	for (c = refname; *c; c++) {
+		if (!isupper(*c) && *c != '-' && *c != '_')
+			return 0;
+	}
+
+	return 1;
+}
+
+static int is_main_pseudoref_syntax(const char *refname)
+{
+	return skip_prefix(refname, "main-worktree/", &refname) &&
+		*refname &&
+		is_pseudoref_syntax(refname);
+}
+
 int expand_ref(struct repository *repo, const char *str, int len,
 	       struct object_id *oid, char **ref)
 {
@@ -716,25 +735,6 @@ static int is_per_worktree_ref(const char *refname)
 	       starts_with(refname, "refs/rewritten/");
 }
 
-static int is_pseudoref_syntax(const char *refname)
-{
-	const char *c;
-
-	for (c = refname; *c; c++) {
-		if (!isupper(*c) && *c != '-' && *c != '_')
-			return 0;
-	}
-
-	return 1;
-}
-
-static int is_main_pseudoref_syntax(const char *refname)
-{
-	return skip_prefix(refname, "main-worktree/", &refname) &&
-		*refname &&
-		is_pseudoref_syntax(refname);
-}
-
 static int is_other_pseudoref_syntax(const char *refname)
 {
 	if (!skip_prefix(refname, "worktrees/", &refname))
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Ævar Arnfjörð Bjarmason]

The refs parsing machinery will first try to parse arbitrary
.git/<name> for a given <name>, before moving onto refs/<name>,
refs/tags/<name> etc. See "ref_rev_parse_rules" in refs.c. Things that
list references such as "for-each-ref" ignore these on the assumption
that they're pseudorefs such as "HEAD".

Thus if you end up in a repository that contains e.g. .git/master the
likes of "checkout" can emit seemingly nonsensical error
messages. E.g. I happened to have a .git/master with a non-commit
SHA-1:

    $ git checkout master
    fatal: Cannot switch branch to a non-commit 'master'

Running "for-each-ref" yields only commits that could match "master",
until I realized I'd ended up with a .git/master file. Before this
we'd ignore it under a general rule that tries to ignore .git/HEAD,
.git/MERGE_HEAD and other non-pseudoref looking refs at the top-level.

Let's help the user in this case by doing a very loose check for
whether the ref name looks like a special pseudoref such as
"HEAD" (i.e. only has upper case, dashes, underbars), and if not issue
a warning:

    $ git rev-parse master
    warning: matched ref .git/master doesn't look like a pseudoref
    c87c83a2e9eb6d309913a0f59389f808024a58f9

I think it's conservative enough to just turn this on by default, but
place it under a configurable option similar to the existing
core.warnAmbiguousRefs. Running the entire test suite with "die"
instead of "warning" passes with this approach.

Our own test suite makes use of a few refs in .git/ that aren't
produced by git itself, e.g. "FOO", "TESTSYMREFTWO" etc. External
tools probably rely on this as well, so I don't think it's viable to
e.g. have a whitelist of them. That list is quite large just fr
git.git, I counted 12 names used in the C code before I abandoned that
approach.

This approach of checking the case of e.g. "master" is not an issue on
case-insensitive filesystems, since we're not checking against the
fs's version of the name, but what the user provided to git on the
command-line.

We are going to match "git rev-parse master" to e.g. .git/MASTER on
those systems, but I think that's also a case the user would like to
be warned about. I once helped a user on OSX with an issue where they
couldn't repeat a merge command on Linux, and it turned out they'd
referred to "HEAD" as "head", which we'd happily resolve to .git/HEAD
without warning on that system. Now we'll warn about that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/core.txt | 11 ++++++++++
 cache.h                       |  1 +
 config.c                      |  5 +++++
 environment.c                 |  1 +
 refs.c                        | 12 ++++++++++
 t/t1430-bad-ref-name.sh       | 41 +++++++++++++++++++++++++++++++++++
 6 files changed, 71 insertions(+)

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 160aacad84b..ecc0757cc51 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -355,6 +355,17 @@ core.warnAmbiguousRefs::
 	If true, Git will warn you if the ref name you passed it is ambiguous
 	and might match multiple refs in the repository. True by default.
 
+core.warnNonPseudoRefs::
+	If true, Git will warn you if the `<ref>` you passed
+	unexpectedly resolves to a top-level ref stored in
+	`.git/<file>` but doesn't look like a pseudoref such as
+	`HEAD`, `MERGE_HEAD` etc. True by default.
++
+These references are ignored by linkgit:for-each-ref[1], but resolved
+by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be
+confusing to have e.g. an errant `.git/mybranch` being confused with
+`.git/refs/heads/mybranch`.
+
 core.compression::
 	An integer -1..9, indicating a default compression level.
 	-1 is the zlib default. 0 means no compression,
diff --git a/cache.h b/cache.h
index 8d279bc1103..1a0cc5e38a3 100644
--- a/cache.h
+++ b/cache.h
@@ -920,6 +920,7 @@ extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int warn_ambiguous_refs;
+extern int warn_non_pseudo_refs;
 extern int warn_on_object_refname_ambiguity;
 extern char *apply_default_whitespace;
 extern char *apply_default_ignorewhitespace;
diff --git a/config.c b/config.c
index 1137bd73aff..6a589a770f4 100644
--- a/config.c
+++ b/config.c
@@ -1212,6 +1212,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.warnnonpseudorefs")) {
+		warn_non_pseudo_refs = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.abbrev")) {
 		if (!value)
 			return config_error_nonbool(var);
diff --git a/environment.c b/environment.c
index bb518c61cd2..85a84eceaf3 100644
--- a/environment.c
+++ b/environment.c
@@ -29,6 +29,7 @@ int assume_unchanged;
 int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
+int warn_non_pseudo_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
 int ref_paranoia = -1;
 int repository_format_precious_objects;
diff --git a/refs.c b/refs.c
index 3ec5dcba0be..634ab64cc9e 100644
--- a/refs.c
+++ b/refs.c
@@ -649,12 +649,19 @@ static int is_main_pseudoref_syntax(const char *refname)
 		is_pseudoref_syntax(refname);
 }
 
+static int is_any_pseudoref_syntax(const char *refname)
+{
+	return is_main_pseudoref_syntax(refname) ||
+		is_pseudoref_syntax(refname);
+}
+
 int expand_ref(struct repository *repo, const char *str, int len,
 	       struct object_id *oid, char **ref)
 {
 	const char **p, *r;
 	int refs_found = 0;
 	struct strbuf fullref = STRBUF_INIT;
+	static int warned_on_non_pseudo_ref;
 
 	*ref = NULL;
 	for (p = ref_rev_parse_rules; *p; p++) {
@@ -669,6 +676,11 @@ int expand_ref(struct repository *repo, const char *str, int len,
 					    fullref.buf, RESOLVE_REF_READING,
 					    this_result, &flag);
 		if (r) {
+			if (warn_non_pseudo_refs &&
+			    !strchr(r, '/') &&
+			    !is_any_pseudoref_syntax(r) &&
+			    !warned_on_non_pseudo_ref++)
+				warning(_(".git/%s doesn't look like a pseudoref"), r);
 			if (!refs_found++)
 				*ref = xstrdup(r);
 			if (!warn_ambiguous_refs)
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index c7878a60edf..782c629e473 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '
 	git show-ref refs/heads/dash
 '
 
+test_expect_success 'warn on non-pseudoref syntax refs in .git/' '
+	test_when_finished "
+		rm -rf .git/mybranch \
+			.git/a-dir \
+			.git/MY-BRANCH_NAME \
+			.git/MY-branch_NAME
+	" &&
+
+	# Setup
+	git rev-parse HEAD >expect &&
+	mkdir .git/a-dir &&
+
+	# We ignore anything with slashes
+	cp expect .git/a-dir/mybranch &&
+	git rev-parse a-dir/mybranch >hash 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect hash &&
+
+	# We ignore upper-case
+	cp expect .git/MY-BRANCH_NAME &&
+	git rev-parse a-dir/mybranch >hash 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect hash &&
+
+	# We ignore mixed-case
+	cp expect .git/MY-branch_NAME &&
+	git rev-parse a-dir/mybranch >hash 2>err &&
+	test_must_be_empty err &&
+	test_cmp expect hash &&
+
+	# We do not ignore lower-case
+	cp expect .git/mybranch &&
+	env GIT_TEST_GETTEXT_POISON=false \
+		git rev-parse mybranch >hash 2>err &&
+	test_cmp expect hash &&
+	grep "like a pseudoref" err &&
+	git -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&
+	test_cmp expect hash &&
+	test_must_be_empty err
+'
+
 test_done
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Eric Sunshine]

On Mon, Dec 14, 2020 at 2:17 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> [...]
> Our own test suite makes use of a few refs in .git/ that aren't
> produced by git itself, e.g. "FOO", "TESTSYMREFTWO" etc. External
> tools probably rely on this as well, so I don't think it's viable to
> e.g. have a whitelist of them. That list is quite large just fr

You want a s/fr/from/ here or something?

> git.git, I counted 12 names used in the C code before I abandoned that
> approach.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Is the Modified-by: footer intentional?

Re: [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Junio C Hamano]

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

> The refs parsing machinery will first try to parse arbitrary
> .git/<name> for a given <name>, before moving onto refs/<name>,
> refs/tags/<name> etc. See "ref_rev_parse_rules" in refs.c. Things that
> list references such as "for-each-ref" ignore these on the assumption
> that they're pseudorefs such as "HEAD".
>
> Thus if you end up in a repository that contains e.g. .git/master the
> likes of "checkout" can emit seemingly nonsensical error
> messages. E.g. I happened to have a .git/master with a non-commit
> SHA-1:
>
>     $ git checkout master
>     fatal: Cannot switch branch to a non-commit 'master'

Or, without even any funny files created in .git to make it
misbehave, "git checkout config" would already give you

    $ git checkout config
    error: pathspec 'config' did not match any file(s) known to git
    $ git checkout config --
    fatal: invalid reference: config

You were unlucky enough to have 40-hex in that garbage file.  How
did you end up with it, I wonder, but anyway, a move to make it
easier to diagnose the situation is very welcome.

> Let's help the user in this case by doing a very loose check for
> whether the ref name looks like a special pseudoref such as
> "HEAD" (i.e. only has upper case, dashes, underbars), and if not issue
> a warning:
>
>     $ git rev-parse master
>     warning: matched ref .git/master doesn't look like a pseudoref
>     c87c83a2e9eb6d309913a0f59389f808024a58f9

With the problem this patch addressed solved, what should happen if
you did this?

    $ git rev-parse refs/heads/master~4 >.git/master
    $ git checkout master

My knee-jerk reaction to the question is that we should still give
the same warning, even though the "checkout" should successfully
detach the HEAD at the commit, to remind you that the name you used
came from may have been resolved to something you did not expect,
and the value you used may not be what you wanted to use.

> I think it's conservative enough to just turn this on by default, but
> place it under a configurable option similar to the existing
> core.warnAmbiguousRefs. Running the entire test suite with "die"
> instead of "warning" passes with this approach.

Sounds sensible.

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

Huh?

> +core.warnNonPseudoRefs::
> +	If true, Git will warn you if the `<ref>` you passed
> +	unexpectedly resolves to a top-level ref stored in
> +	`.git/<file>` but doesn't look like a pseudoref such as
> +	`HEAD`, `MERGE_HEAD` etc. True by default.

I'd really prefer to see us to think, design and describe in terms
of a more generic ref API, when we do not have to limit us to the
behaviour only with files backend.  Reading the file `.git/<ref>` on
the filesystem happens to be one way to resolve a ref to its value
(i.e. reference to an object), but even with different ref backends,
we want 'master' that sits next to 'refs/heads/master' to trigger
this warning.  So

	... if the `<ref>` you passed refers to a <ref> outside the
	`refs/{heads,tags,...}/` hierarchy but does not look like
	...

or something like that, perhaps.

> ++
> +These references are ignored by linkgit:for-each-ref[1], but resolved
> +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be
> +confusing to have e.g. an errant `.git/mybranch` being confused with
> +`.git/refs/heads/mybranch`.

Here, ".git/mybranch" and ".git/refs/heads/mybranch" are good as
examples.  I just want to avoid tying the main description to the
files backend.

> diff --git a/refs.c b/refs.c
> index 3ec5dcba0be..634ab64cc9e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -649,12 +649,19 @@ static int is_main_pseudoref_syntax(const char *refname)
>  		is_pseudoref_syntax(refname);
>  }
>  
> +static int is_any_pseudoref_syntax(const char *refname)
> +{
> +	return is_main_pseudoref_syntax(refname) ||
> +		is_pseudoref_syntax(refname);
> +}

Hmph, why is this needed, I wonder.

"git show main-worktree/master" is specific enough to tell us that
the user did not mean "git show main-worktree/refs/heads/master",
no?  If so, then wouldn't it be sufficient to check only with
is_pseudoref_syntax() and nothing else?  I may probably missing
something to make me convince is_main_pseudoref_syntax() matters
here.

>  int expand_ref(struct repository *repo, const char *str, int len,
>  	       struct object_id *oid, char **ref)
>  {
>  	const char **p, *r;
>  	int refs_found = 0;
>  	struct strbuf fullref = STRBUF_INIT;
> +	static int warned_on_non_pseudo_ref;
>  
>  	*ref = NULL;
>  	for (p = ref_rev_parse_rules; *p; p++) {
> @@ -669,6 +676,11 @@ int expand_ref(struct repository *repo, const char *str, int len,
>  					    fullref.buf, RESOLVE_REF_READING,
>  					    this_result, &flag);
>  		if (r) {
> +			if (warn_non_pseudo_refs &&
> +			    !strchr(r, '/') &&
> +			    !is_any_pseudoref_syntax(r) &&
> +			    !warned_on_non_pseudo_ref++)
> +				warning(_(".git/%s doesn't look like a pseudoref"), r);

I do not see much point in reporting only the first one.

Isn't the conditional "if (r)" sufficiently limiting the ref only to
those that the user showed immediate interest in?  In other words,
even if there were .git/foo and .git/bar, both of which happens to
contain something that looks like an object name, "git show foo"
would cause this code to warn only about .git/foo and .git/bar, no?

If there are more than one hits, why shouldn't we report all of
them?

Or is this some performance thing (i.e. there only can be one hit,
so repeated calls to strchr() and is_any_pseudoref_syntax() after
seeing one hit would all be wasteful)?  If so, "have we warned? if
so skip the remainder of checking, as we won't warn" should be the
first in the &&-chain.

Puzzled.

Thanks.

Re: [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 14 2020, Junio C Hamano wrote:

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

I think I was using that to test my %(trailers) patches & didn't
notice. Sorry.

As for the rest of the feedback thanks & I'll address it.

I'm planning to re-roll this but given that we're in the rc window & the
time I have to re-roll this in the coming week(s) let's put this series
on hold/eject it (in case you were planning to put it into "seen") until
we start post-v2.30 work.