{"thread":{"id":"54808","subject":"[PATCH 1/2] refs: move is_pseudoref_syntax() earlier in the file","startedAt":"2020-12-10T12:54:21Z","lastAt":"2020-12-15T23:08:31Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"411992","messageId":"20201210125321.19456-1-avarab@gmail.com","threadId":"54808","inReplyTo":null,"subject":"[PATCH 1/2] refs: move is_pseudoref_syntax() earlier in the file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-10T12:53:20Z","receivedAt":"2020-12-10T12:54:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This move is needed by a subsequent change to the expand_ref()\nfunction, which will make use of these functions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n refs.c | 38 +++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 392f0bbf68b..3ec5dcba0be 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -630,6 +630,25 @@ int repo_dwim_ref(struct repository *r, const char *str, int len,\n \treturn refs_found;\n }\n \n+static int is_pseudoref_syntax(const char *refname)\n+{\n+\tconst char *c;\n+\n+\tfor (c = refname; *c; c++) {\n+\t\tif (!isupper(*c) && *c != '-' && *c != '_')\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n+static int is_main_pseudoref_syntax(const char *refname)\n+{\n+\treturn skip_prefix(refname, \"main-worktree/\", &refname) &&\n+\t\t*refname &&\n+\t\tis_pseudoref_syntax(refname);\n+}\n+\n int expand_ref(struct repository *repo, const char *str, int len,\n \t       struct object_id *oid, char **ref)\n {\n@@ -716,25 +735,6 @@ static int is_per_worktree_ref(const char *refname)\n \t       starts_with(refname, \"refs/rewritten/\");\n }\n \n-static int is_pseudoref_syntax(const char *refname)\n-{\n-\tconst char *c;\n-\n-\tfor (c = refname; *c; c++) {\n-\t\tif (!isupper(*c) && *c != '-' && *c != '_')\n-\t\t\treturn 0;\n-\t}\n-\n-\treturn 1;\n-}\n-\n-static int is_main_pseudoref_syntax(const char *refname)\n-{\n-\treturn skip_prefix(refname, \"main-worktree/\", &refname) &&\n-\t\t*refname &&\n-\t\tis_pseudoref_syntax(refname);\n-}\n-\n static int is_other_pseudoref_syntax(const char *refname)\n {\n \tif (!skip_prefix(refname, \"worktrees/\", &refname))\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411993","messageId":"20201210125321.19456-2-avarab@gmail.com","threadId":"54808","inReplyTo":"20201210125321.19456-1-avarab@gmail.com","subject":"[PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-10T12:53:21Z","receivedAt":"2020-12-10T12:54:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The refs parsing machinery will first try to parse arbitrary\n.git/<name> for a given <name>, before moving onto refs/<name>,\nrefs/tags/<name> etc. See \"ref_rev_parse_rules\" in refs.c, but\ne.g. \"for-each-ref\" and other things that list references ignore these\nancient-style refs.\n\nThus if you end up in a repository that contains e.g. .git/master the\nlikes of \"checkout\" can emit seemingly nonsensical error\nmessages. E.g. I happened to have a .git/master with a non-commit\nSHA-1:\n\n    $ git checkout master\n    fatal: Cannot switch branch to a non-commit 'master'\n\nRunning \"for-each-ref\" yields only commits that could match \"master\",\nuntil I realized I'd ended up with a .git/master file. Before this\nwe'd ignore it under a general rule that tries to ignore .git/HEAD,\n.git/MERGE_HEAD and other non-pseudoref looking refs at the top-level.\n\nLet's help the user in this case by doing a very loose check for\nwhether the ref name looks like a pseudoref such as \"HEAD\" (i.e. only\nhas upper case, dashes, underbars), and if not issue a warning:\n\n    $ git rev-parse master\n    warning: matched ref .git/master doesn't look like a pseudoref\n    c87c83a2e9eb6d309913a0f59389f808024a58f9\n\nI think it's conservative enough to just turn this on by default, but\nplace it under a configurable option similar to the existing\ncore.warnAmbiguousRefs. Running the entire test suite with \"die\"\ninstead of \"warning\" passes with this approach.\n\nOur own test suite makes use of a few refs in .git/ that aren't\nproduced by git itself, e.g. \"FOO\", \"TESTSYMREFTWO\" etc, external\ntools probably rely on this as well, so I don't think it's viable to\ne.g. have a whitelist of \"HEAD\", \"MERGE_HEAD\" etc. As an aside that\nlist is quite large, I counted 12 names used in the C code before I\nabandoned that approach.\n\nThis approach of checking the case of e.g. \"master\" is not an issue on\ncase-insensitive filesystems, since we're not checking against the\nfs's version of the name, but what the user provided to git on the\ncommand-line.\n\nWe are going to match \"git rev-parse master\" to e.g. .git/MASTER on\nthose systems, but I think that's also a case the user would like to\nbe warned about. I once helped a user on OSX with an issue where they\ncouldn't repeat a merge command on Linux, and it turned out they'd\nreferred to \"HEAD\" as \"head\", which we'd happily resolve to .git/HEAD\nwithout warning on that system. Now we'll warn about that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/config/core.txt | 11 ++++++++++\n cache.h                       |  1 +\n config.c                      |  5 +++++\n environment.c                 |  1 +\n refs.c                        | 20 +++++++++++++++++\n t/t1430-bad-ref-name.sh       | 41 +++++++++++++++++++++++++++++++++++\n 6 files changed, 79 insertions(+)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 160aacad84b..535a7012156 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -355,6 +355,17 @@ core.warnAmbiguousRefs::\n \tIf true, Git will warn you if the ref name you passed it is ambiguous\n \tand might match multiple refs in the repository. True by default.\n \n+core.warnNonPseudoRefs::\n+\tIf true, Git will warn you if the `<ref>` you passed\n+\tunexpectedly resolves to a top-level ref stored in\n+\t`.git/<file>` but doesn't look like a pseudoref such as\n+\t`HEAD`, `MERGE_HEAD` etc. True by default.\n++\n+These references are ignored by linkgit:for-each-ref[1], but resolved\n+by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be\n+confusing to have e.g. an errant `.git/master` being confused with\n+`.git/refs/heads/master`.\n+\n core.compression::\n \tAn integer -1..9, indicating a default compression level.\n \t-1 is the zlib default. 0 means no compression,\ndiff --git a/cache.h b/cache.h\nindex 8d279bc1103..1a0cc5e38a3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -920,6 +920,7 @@ extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\n extern int warn_ambiguous_refs;\n+extern int warn_non_pseudo_refs;\n extern int warn_on_object_refname_ambiguity;\n extern char *apply_default_whitespace;\n extern char *apply_default_ignorewhitespace;\ndiff --git a/config.c b/config.c\nindex 1137bd73aff..6a589a770f4 100644\n--- a/config.c\n+++ b/config.c\n@@ -1212,6 +1212,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.warnnonpseudorefs\")) {\n+\t\twarn_non_pseudo_refs = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.abbrev\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\ndiff --git a/environment.c b/environment.c\nindex bb518c61cd2..85a84eceaf3 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -29,6 +29,7 @@ int assume_unchanged;\n int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n+int warn_non_pseudo_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n int repository_format_precious_objects;\ndiff --git a/refs.c b/refs.c\nindex 3ec5dcba0be..0beeb3dded8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -649,12 +649,19 @@ static int is_main_pseudoref_syntax(const char *refname)\n \t\tis_pseudoref_syntax(refname);\n }\n \n+static int is_any_pseudoref_syntax(const char *refname)\n+{\n+\treturn is_main_pseudoref_syntax(refname) ||\n+\t\tis_pseudoref_syntax(refname);\n+}\n+\n int expand_ref(struct repository *repo, const char *str, int len,\n \t       struct object_id *oid, char **ref)\n {\n \tconst char **p, *r;\n \tint refs_found = 0;\n \tstruct strbuf fullref = STRBUF_INIT;\n+\tstatic int warned_on_non_pseudo_ref;\n \n \t*ref = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n@@ -669,6 +676,19 @@ int expand_ref(struct repository *repo, const char *str, int len,\n \t\t\t\t\t    fullref.buf, RESOLVE_REF_READING,\n \t\t\t\t\t    this_result, &flag);\n \t\tif (r) {\n+\t\t\tif (warn_non_pseudo_refs &&\n+\t\t\t    !starts_with(fullref.buf, \"refs/\") &&\n+\t\t\t    !starts_with(r, \"refs/\") &&\n+\t\t\t    !strchr(r, '/') &&\n+\t\t\t    !is_any_pseudoref_syntax(r) &&\n+\t\t\t    !warned_on_non_pseudo_ref++) {\n+\t\t\t\t/*\n+\t\t\t\t * TRANSLATORS: The 1st argument is\n+\t\t\t\t * e.g. \"master\", and the 2nd can be\n+\t\t\t\t * e.g. \"master~10\".\n+\t\t\t\t */\n+\t\t\t\twarning(_(\"matched ref name .git/%s doesn't look like a pseudoref\"), r);\n+\t\t\t}\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n \t\t\tif (!warn_ambiguous_refs)\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex c7878a60edf..2c8c2c15c40 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '\n \tgit show-ref refs/heads/dash\n '\n \n+test_expect_success 'warn on non-pseudoref syntax refs in .git/' '\n+\ttest_when_finished \"\n+\t\trm -f .git/mybranch &&\n+\t\trm -rf .git/a-dir &&\n+\t\trm -rf .git/MY-BRANCH_NAME &&\n+\t\trm -rf .git/MY-branch_NAME\n+\t\" &&\n+\n+\t# Setup\n+\tgit rev-parse master >expect &&\n+\n+\t# We ignore anything with slashes\n+\tmkdir .git/a-dir &&\n+\tcp expect .git/a-dir/mybranch &&\n+\tgit rev-parse a-dir/mybranch >hash 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect hash &&\n+\n+\t# We ignore upper-case\n+\tcp expect .git/MY-BRANCH_NAME &&\n+\tgit rev-parse a-dir/mybranch >hash 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect hash &&\n+\n+\t# We ignore mixed-case\n+\tcp expect .git/MY-branch_NAME &&\n+\tgit rev-parse a-dir/mybranch >hash 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect hash &&\n+\n+\t# We do not ignore lower-case\n+\tcp expect .git/mybranch &&\n+\tgit rev-parse mybranch >hash 2>err &&\n+\ttest_cmp expect hash &&\n+\tGIT_TEST_GETTEXT_POISON=false grep \"like a pseudoref\" err &&\n+\tgit -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&\n+\ttest_cmp expect hash &&\n+\ttest_must_be_empty err &&\n+\trm .git/mybranch\n+'\n+\n test_done\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"412002","messageId":"CAPig+cQpB3B+Ywc1aFd7QMrqVKy29VmNKJfzcFeSCYz=mwO0Hw@mail.gmail.com","threadId":"54808","inReplyTo":"20201210125321.19456-2-avarab@gmail.com","subject":"Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-10T14:39:07Z","receivedAt":"2020-12-10T15:43:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> [...]\n> Let's help the user in this case by doing a very loose check for\n> whether the ref name looks like a pseudoref such as \"HEAD\" (i.e. only\n> has upper case, dashes, underbars), and if not issue a warning:\n> [...]\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> @@ -355,6 +355,17 @@ core.warnAmbiguousRefs::\n> +core.warnNonPseudoRefs::\n> +       If true, Git will warn you if the `<ref>` you passed\n> +       unexpectedly resolves to a top-level ref stored in\n> +       `.git/<file>` but doesn't look like a pseudoref such as\n> +       `HEAD`, `MERGE_HEAD` etc. True by default.\n> ++\n> +These references are ignored by linkgit:for-each-ref[1], but resolved\n> +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be\n> +confusing to have e.g. an errant `.git/master` being confused with\n> +`.git/refs/heads/master`.\n\nDscho has been submitting patches lately to eradicate the word\n\"master\" from the project source.\n\n> diff --git a/refs.c b/refs.c\n> @@ -669,6 +676,19 @@ int expand_ref(struct repository *repo, const char *str, int len,\n>                 if (r) {\n> +                       if (warn_non_pseudo_refs &&\n> +                           !starts_with(fullref.buf, \"refs/\") &&\n> +                           !starts_with(r, \"refs/\") &&\n> +                           !strchr(r, '/') &&\n> +                           !is_any_pseudoref_syntax(r) &&\n> +                           !warned_on_non_pseudo_ref++) {\n> +                               /*\n> +                                * TRANSLATORS: The 1st argument is\n> +                                * e.g. \"master\", and the 2nd can be\n> +                                * e.g. \"master~10\".\n> +                                */\n> +                               warning(_(\"matched ref name .git/%s doesn't look like a pseudoref\"), r);\n\nThe TRANSLATORS comment talks about two arguments, but I see only one.\n\nDoes the \"matched ref name\" part add any value? I would find the\nwarning just as helpful without it:\n\n    .git/blork doesn't look like a pseudoref\n\n> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n> @@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '\n> +test_expect_success 'warn on non-pseudoref syntax refs in .git/' '\n> +       test_when_finished \"\n> +               rm -f .git/mybranch &&\n> +               rm -rf .git/a-dir &&\n> +               rm -rf .git/MY-BRANCH_NAME &&\n> +               rm -rf .git/MY-branch_NAME\n> +       \" &&\n\nNit: These could all be removed with a single `rm -rf`:\n\n    rm -rf .git/mybranch .git/a-dir ...\n\n> +       # We do not ignore lower-case\n> +       cp expect .git/mybranch &&\n> +       git rev-parse mybranch >hash 2>err &&\n> +       test_cmp expect hash &&\n> +       GIT_TEST_GETTEXT_POISON=false grep \"like a pseudoref\" err &&\n\nWhat is the purpose of assigning GIT_TEST_GETTEXT_POISON here?\n\n> +       git -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&\n> +       test_cmp expect hash &&\n> +       test_must_be_empty err &&\n> +       rm .git/mybranch\n> +'\n"},{"id":"412012","messageId":"87sg8dwglk.fsf@evledraar.gmail.com","threadId":"54808","inReplyTo":"CAPig+cQpB3B+Ywc1aFd7QMrqVKy29VmNKJfzcFeSCYz=mwO0Hw@mail.gmail.com","subject":"Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-10T20:28:39Z","receivedAt":"2020-12-10T20:30:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 10 2020, Eric Sunshine wrote:\n\n> On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> [...]\n>> Let's help the user in this case by doing a very loose check for\n>> whether the ref name looks like a pseudoref such as \"HEAD\" (i.e. only\n>> has upper case, dashes, underbars), and if not issue a warning:\n>> [...]\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n>> @@ -355,6 +355,17 @@ core.warnAmbiguousRefs::\n>> +core.warnNonPseudoRefs::\n>> +       If true, Git will warn you if the `<ref>` you passed\n>> +       unexpectedly resolves to a top-level ref stored in\n>> +       `.git/<file>` but doesn't look like a pseudoref such as\n>> +       `HEAD`, `MERGE_HEAD` etc. True by default.\n>> ++\n>> +These references are ignored by linkgit:for-each-ref[1], but resolved\n>> +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be\n>> +confusing to have e.g. an errant `.git/master` being confused with\n>> +`.git/refs/heads/master`.\n>\n> Dscho has been submitting patches lately to eradicate the word\n> \"master\" from the project source.\n\nMuscle memory dies hard, will fix it in the reroll.\n\n>> diff --git a/refs.c b/refs.c\n>> @@ -669,6 +676,19 @@ int expand_ref(struct repository *repo, const char *str, int len,\n>>                 if (r) {\n>> +                       if (warn_non_pseudo_refs &&\n>> +                           !starts_with(fullref.buf, \"refs/\") &&\n>> +                           !starts_with(r, \"refs/\") &&\n>> +                           !strchr(r, '/') &&\n>> +                           !is_any_pseudoref_syntax(r) &&\n>> +                           !warned_on_non_pseudo_ref++) {\n>> +                               /*\n>> +                                * TRANSLATORS: The 1st argument is\n>> +                                * e.g. \"master\", and the 2nd can be\n>> +                                * e.g. \"master~10\".\n>> +                                */\n>> +                               warning(_(\"matched ref name .git/%s doesn't look like a pseudoref\"), r);\n>\n> The TRANSLATORS comment talks about two arguments, but I see only one.\n>\n> Does the \"matched ref name\" part add any value? I would find the\n> warning just as helpful without it:\n>\n>     .git/blork doesn't look like a pseudoref\n\nYeah that's much better, the TRANSLATORS comment is incorrect, rebased\nout an earlier version where it took two, didn't notice...\n\n>> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\n>> @@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '\n>> +test_expect_success 'warn on non-pseudoref syntax refs in .git/' '\n>> +       test_when_finished \"\n>> +               rm -f .git/mybranch &&\n>> +               rm -rf .git/a-dir &&\n>> +               rm -rf .git/MY-BRANCH_NAME &&\n>> +               rm -rf .git/MY-branch_NAME\n>> +       \" &&\n>\n> Nit: These could all be removed with a single `rm -rf`:\n>\n>     rm -rf .git/mybranch .git/a-dir ...\n\nWill fix.\n\n>> +       # We do not ignore lower-case\n>> +       cp expect .git/mybranch &&\n>> +       git rev-parse mybranch >hash 2>err &&\n>> +       test_cmp expect hash &&\n>> +       GIT_TEST_GETTEXT_POISON=false grep \"like a pseudoref\" err &&\n>\n> What is the purpose of assigning GIT_TEST_GETTEXT_POISON here?\n\nSince 6cdccfce1e0 (i18n: make GETTEXT_POISON a runtime option,\n2018-11-08) we haven't needed to use the C_LOCALE_OUTPUT prerequisite\nfor any new code, since we can just turn the poisoning off.\n\nI think we should just slowly refactor things away from that\nprerequisite and test_i18ngrep, which were only needed because it used\nto be a compile-time switch, but I haven't gotter around to that\nrefactoring.\n\nIn liue of that I think it makes more sense to always run the full test\nif possible, no matter what the GIT_TEST_* mode is.\n\n>> +       git -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&\n>> +       test_cmp expect hash &&\n>> +       test_must_be_empty err &&\n>> +       rm .git/mybranch\n>> +'\n\n"},{"id":"412013","messageId":"CAPig+cSph9EcnGDUBjWsvR3yfbydK=mOMPmCEas=YC6bUJS=zA@mail.gmail.com","threadId":"54808","inReplyTo":"87sg8dwglk.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-10T20:47:30Z","receivedAt":"2020-12-10T20:48:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 10, 2020 at 3:29 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Thu, Dec 10 2020, Eric Sunshine wrote:\n> > On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >> +       GIT_TEST_GETTEXT_POISON=false grep \"like a pseudoref\" err &&\n> >\n> > What is the purpose of assigning GIT_TEST_GETTEXT_POISON here?\n>\n> Since 6cdccfce1e0 (i18n: make GETTEXT_POISON a runtime option,\n> 2018-11-08) we haven't needed to use the C_LOCALE_OUTPUT prerequisite\n> for any new code, since we can just turn the poisoning off.\n>\n> I think we should just slowly refactor things away from that\n> prerequisite and test_i18ngrep, which were only needed because it used\n> to be a compile-time switch, but I haven't gotter around to that\n> refactoring.\n>\n> In liue of that I think it makes more sense to always run the full test\n> if possible, no matter what the GIT_TEST_* mode is.\n\nI must be missing something. I've looked at 6cdccfce1e0 but I still\ndon't see how or why `GIT_TEST_GETTEXT_POISON=false` could affect the\nsimple `grep` invocation being done by this test. I could understand\nif GIT_TEST_GETTEXT_POISON was applied to the invocation of a Git\ncommand, but that's not the case here.\n\n(I also notice that 6cdccfce1e0 only checks whether\nGIT_TEST_GETTEXT_POISON is empty or not -- and the changes in\n6cdccfce1e0 set GIT_TEST_GETTEXT_POISON to an empty value rather than\nto \"false\", so I find myself doubly confused by this application of\nGIT_TEST_GETTEXT_POISON to `grep`.)\n"},{"id":"412023","messageId":"87mtylwbjs.fsf@evledraar.gmail.com","threadId":"54808","inReplyTo":"CAPig+cSph9EcnGDUBjWsvR3yfbydK=mOMPmCEas=YC6bUJS=zA@mail.gmail.com","subject":"Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-10T22:17:43Z","receivedAt":"2020-12-10T23:08:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 10 2020, Eric Sunshine wrote:\n\n> On Thu, Dec 10, 2020 at 3:29 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>> On Thu, Dec 10 2020, Eric Sunshine wrote:\n>> > On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason\n>> > <avarab@gmail.com> wrote:\n>> >> +       GIT_TEST_GETTEXT_POISON=false grep \"like a pseudoref\" err &&\n>> >\n>> > What is the purpose of assigning GIT_TEST_GETTEXT_POISON here?\n>>\n>> Since 6cdccfce1e0 (i18n: make GETTEXT_POISON a runtime option,\n>> 2018-11-08) we haven't needed to use the C_LOCALE_OUTPUT prerequisite\n>> for any new code, since we can just turn the poisoning off.\n>>\n>> I think we should just slowly refactor things away from that\n>> prerequisite and test_i18ngrep, which were only needed because it used\n>> to be a compile-time switch, but I haven't gotter around to that\n>> refactoring.\n>>\n>> In liue of that I think it makes more sense to always run the full test\n>> if possible, no matter what the GIT_TEST_* mode is.\n>\n> I must be missing something. I've looked at 6cdccfce1e0 but I still\n> don't see how or why `GIT_TEST_GETTEXT_POISON=false` could affect the\n> simple `grep` invocation being done by this test. I could understand\n> if GIT_TEST_GETTEXT_POISON was applied to the invocation of a Git\n> command, but that's not the case here.\n>\n> (I also notice that 6cdccfce1e0 only checks whether\n> GIT_TEST_GETTEXT_POISON is empty or not -- and the changes in\n> 6cdccfce1e0 set GIT_TEST_GETTEXT_POISON to an empty value rather than\n> to \"false\", so I find myself doubly confused by this application of\n> GIT_TEST_GETTEXT_POISON to `grep`.)\n\nYou're not missing something, but it seems I need to step away from the\ncomputer for the day. I managed to write this & reply to your E-Mail\nwithout noticing I'd put the GIT_TEST[...] env variable in front of the\nwrong command. It should indeed be in front of the rev-parse above.\n"},{"id":"412189","messageId":"20201214191700.16405-3-avarab@gmail.com","threadId":"54808","inReplyTo":"20201210125321.19456-2-avarab@gmail.com","subject":"[PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-14T19:17:00Z","receivedAt":"2020-12-14T19:18:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The refs parsing machinery will first try to parse arbitrary\n.git/<name> for a given <name>, before moving onto refs/<name>,\nrefs/tags/<name> etc. See \"ref_rev_parse_rules\" in refs.c. Things that\nlist references such as \"for-each-ref\" ignore these on the assumption\nthat they're pseudorefs such as \"HEAD\".\n\nThus if you end up in a repository that contains e.g. .git/master the\nlikes of \"checkout\" can emit seemingly nonsensical error\nmessages. E.g. I happened to have a .git/master with a non-commit\nSHA-1:\n\n    $ git checkout master\n    fatal: Cannot switch branch to a non-commit 'master'\n\nRunning \"for-each-ref\" yields only commits that could match \"master\",\nuntil I realized I'd ended up with a .git/master file. Before this\nwe'd ignore it under a general rule that tries to ignore .git/HEAD,\n.git/MERGE_HEAD and other non-pseudoref looking refs at the top-level.\n\nLet's help the user in this case by doing a very loose check for\nwhether the ref name looks like a special pseudoref such as\n\"HEAD\" (i.e. only has upper case, dashes, underbars), and if not issue\na warning:\n\n    $ git rev-parse master\n    warning: matched ref .git/master doesn't look like a pseudoref\n    c87c83a2e9eb6d309913a0f59389f808024a58f9\n\nI think it's conservative enough to just turn this on by default, but\nplace it under a configurable option similar to the existing\ncore.warnAmbiguousRefs. Running the entire test suite with \"die\"\ninstead of \"warning\" passes with this approach.\n\nOur own test suite makes use of a few refs in .git/ that aren't\nproduced by git itself, e.g. \"FOO\", \"TESTSYMREFTWO\" etc. External\ntools probably rely on this as well, so I don't think it's viable to\ne.g. have a whitelist of them. That list is quite large just fr\ngit.git, I counted 12 names used in the C code before I abandoned that\napproach.\n\nThis approach of checking the case of e.g. \"master\" is not an issue on\ncase-insensitive filesystems, since we're not checking against the\nfs's version of the name, but what the user provided to git on the\ncommand-line.\n\nWe are going to match \"git rev-parse master\" to e.g. .git/MASTER on\nthose systems, but I think that's also a case the user would like to\nbe warned about. I once helped a user on OSX with an issue where they\ncouldn't repeat a merge command on Linux, and it turned out they'd\nreferred to \"HEAD\" as \"head\", which we'd happily resolve to .git/HEAD\nwithout warning on that system. Now we'll warn about that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nModified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/config/core.txt | 11 ++++++++++\n cache.h                       |  1 +\n config.c                      |  5 +++++\n environment.c                 |  1 +\n refs.c                        | 12 ++++++++++\n t/t1430-bad-ref-name.sh       | 41 +++++++++++++++++++++++++++++++++++\n 6 files changed, 71 insertions(+)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 160aacad84b..ecc0757cc51 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -355,6 +355,17 @@ core.warnAmbiguousRefs::\n \tIf true, Git will warn you if the ref name you passed it is ambiguous\n \tand might match multiple refs in the repository. True by default.\n \n+core.warnNonPseudoRefs::\n+\tIf true, Git will warn you if the `<ref>` you passed\n+\tunexpectedly resolves to a top-level ref stored in\n+\t`.git/<file>` but doesn't look like a pseudoref such as\n+\t`HEAD`, `MERGE_HEAD` etc. True by default.\n++\n+These references are ignored by linkgit:for-each-ref[1], but resolved\n+by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be\n+confusing to have e.g. an errant `.git/mybranch` being confused with\n+`.git/refs/heads/mybranch`.\n+\n core.compression::\n \tAn integer -1..9, indicating a default compression level.\n \t-1 is the zlib default. 0 means no compression,\ndiff --git a/cache.h b/cache.h\nindex 8d279bc1103..1a0cc5e38a3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -920,6 +920,7 @@ extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\n extern int warn_ambiguous_refs;\n+extern int warn_non_pseudo_refs;\n extern int warn_on_object_refname_ambiguity;\n extern char *apply_default_whitespace;\n extern char *apply_default_ignorewhitespace;\ndiff --git a/config.c b/config.c\nindex 1137bd73aff..6a589a770f4 100644\n--- a/config.c\n+++ b/config.c\n@@ -1212,6 +1212,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.warnnonpseudorefs\")) {\n+\t\twarn_non_pseudo_refs = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.abbrev\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\ndiff --git a/environment.c b/environment.c\nindex bb518c61cd2..85a84eceaf3 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -29,6 +29,7 @@ int assume_unchanged;\n int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n+int warn_non_pseudo_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n int repository_format_precious_objects;\ndiff --git a/refs.c b/refs.c\nindex 3ec5dcba0be..634ab64cc9e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -649,12 +649,19 @@ static int is_main_pseudoref_syntax(const char *refname)\n \t\tis_pseudoref_syntax(refname);\n }\n \n+static int is_any_pseudoref_syntax(const char *refname)\n+{\n+\treturn is_main_pseudoref_syntax(refname) ||\n+\t\tis_pseudoref_syntax(refname);\n+}\n+\n int expand_ref(struct repository *repo, const char *str, int len,\n \t       struct object_id *oid, char **ref)\n {\n \tconst char **p, *r;\n \tint refs_found = 0;\n \tstruct strbuf fullref = STRBUF_INIT;\n+\tstatic int warned_on_non_pseudo_ref;\n \n \t*ref = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n@@ -669,6 +676,11 @@ int expand_ref(struct repository *repo, const char *str, int len,\n \t\t\t\t\t    fullref.buf, RESOLVE_REF_READING,\n \t\t\t\t\t    this_result, &flag);\n \t\tif (r) {\n+\t\t\tif (warn_non_pseudo_refs &&\n+\t\t\t    !strchr(r, '/') &&\n+\t\t\t    !is_any_pseudoref_syntax(r) &&\n+\t\t\t    !warned_on_non_pseudo_ref++)\n+\t\t\t\twarning(_(\".git/%s doesn't look like a pseudoref\"), r);\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n \t\t\tif (!warn_ambiguous_refs)\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex c7878a60edf..782c629e473 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -374,4 +374,45 @@ test_expect_success 'branch -m can rename refs/heads/-dash' '\n \tgit show-ref refs/heads/dash\n '\n \n+test_expect_success 'warn on non-pseudoref syntax refs in .git/' '\n+\ttest_when_finished \"\n+\t\trm -rf .git/mybranch \\\n+\t\t\t.git/a-dir \\\n+\t\t\t.git/MY-BRANCH_NAME \\\n+\t\t\t.git/MY-branch_NAME\n+\t\" &&\n+\n+\t# Setup\n+\tgit rev-parse HEAD >expect &&\n+\tmkdir .git/a-dir &&\n+\n+\t# We ignore anything with slashes\n+\tcp expect .git/a-dir/mybranch &&\n+\tgit rev-parse a-dir/mybranch >hash 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect hash &&\n+\n+\t# We ignore upper-case\n+\tcp expect .git/MY-BRANCH_NAME &&\n+\tgit rev-parse a-dir/mybranch >hash 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect hash &&\n+\n+\t# We ignore mixed-case\n+\tcp expect .git/MY-branch_NAME &&\n+\tgit rev-parse a-dir/mybranch >hash 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect hash &&\n+\n+\t# We do not ignore lower-case\n+\tcp expect .git/mybranch &&\n+\tenv GIT_TEST_GETTEXT_POISON=false \\\n+\t\tgit rev-parse mybranch >hash 2>err &&\n+\ttest_cmp expect hash &&\n+\tgrep \"like a pseudoref\" err &&\n+\tgit -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&\n+\ttest_cmp expect hash &&\n+\ttest_must_be_empty err\n+'\n+\n test_done\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"412190","messageId":"20201214191700.16405-2-avarab@gmail.com","threadId":"54808","inReplyTo":"20201210125321.19456-2-avarab@gmail.com","subject":"[PATCH v2 1/2] refs: move is_pseudoref_syntax() earlier in the file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-14T19:16:59Z","receivedAt":"2020-12-14T19:19:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This move is needed by a subsequent change to the expand_ref()\nfunction, which will make use of these functions.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n refs.c | 38 +++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 392f0bbf68b..3ec5dcba0be 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -630,6 +630,25 @@ int repo_dwim_ref(struct repository *r, const char *str, int len,\n \treturn refs_found;\n }\n \n+static int is_pseudoref_syntax(const char *refname)\n+{\n+\tconst char *c;\n+\n+\tfor (c = refname; *c; c++) {\n+\t\tif (!isupper(*c) && *c != '-' && *c != '_')\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n+static int is_main_pseudoref_syntax(const char *refname)\n+{\n+\treturn skip_prefix(refname, \"main-worktree/\", &refname) &&\n+\t\t*refname &&\n+\t\tis_pseudoref_syntax(refname);\n+}\n+\n int expand_ref(struct repository *repo, const char *str, int len,\n \t       struct object_id *oid, char **ref)\n {\n@@ -716,25 +735,6 @@ static int is_per_worktree_ref(const char *refname)\n \t       starts_with(refname, \"refs/rewritten/\");\n }\n \n-static int is_pseudoref_syntax(const char *refname)\n-{\n-\tconst char *c;\n-\n-\tfor (c = refname; *c; c++) {\n-\t\tif (!isupper(*c) && *c != '-' && *c != '_')\n-\t\t\treturn 0;\n-\t}\n-\n-\treturn 1;\n-}\n-\n-static int is_main_pseudoref_syntax(const char *refname)\n-{\n-\treturn skip_prefix(refname, \"main-worktree/\", &refname) &&\n-\t\t*refname &&\n-\t\tis_pseudoref_syntax(refname);\n-}\n-\n static int is_other_pseudoref_syntax(const char *refname)\n {\n \tif (!skip_prefix(refname, \"worktrees/\", &refname))\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"412191","messageId":"20201214191700.16405-1-avarab@gmail.com","threadId":"54808","inReplyTo":"20201210125321.19456-2-avarab@gmail.com","subject":"[PATCH v2 0/2] warn on non-pseudoref looking .git/<file> refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-14T19:16:58Z","receivedAt":"2020-12-14T19:19:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Now with a cover letter to show the range-diff. I addressed all of\nEric's comments, and found some other minor issues myself. The check\nfor whether we need to warn has also been simplified, we just needed\nto check if we have \"/\" in the resolved \"r\" variable.\n\nÆvar Arnfjörð Bjarmason (2):\n  refs: move is_pseudoref_syntax() earlier in the file\n  refs: warn on non-pseudoref looking .git/<file> refs\n\n Documentation/config/core.txt | 11 ++++++++\n cache.h                       |  1 +\n config.c                      |  5 ++++\n environment.c                 |  1 +\n refs.c                        | 50 ++++++++++++++++++++++-------------\n t/t1430-bad-ref-name.sh       | 41 ++++++++++++++++++++++++++++\n 6 files changed, 90 insertions(+), 19 deletions(-)\n\nRange-diff:\n-:  ----------- > 1:  e86e55f2828 refs: move is_pseudoref_syntax() earlier in the file\n1:  cad73aba664 ! 2:  0573da1d381 refs: warn on non-pseudoref looking .git/<file> refs\n    @@ Commit message\n     \n         The refs parsing machinery will first try to parse arbitrary\n         .git/<name> for a given <name>, before moving onto refs/<name>,\n    -    refs/tags/<name> etc. See \"ref_rev_parse_rules\" in refs.c, but\n    -    e.g. \"for-each-ref\" and other things that list references ignore these\n    -    ancient-style refs.\n    +    refs/tags/<name> etc. See \"ref_rev_parse_rules\" in refs.c. Things that\n    +    list references such as \"for-each-ref\" ignore these on the assumption\n    +    that they're pseudorefs such as \"HEAD\".\n     \n         Thus if you end up in a repository that contains e.g. .git/master the\n         likes of \"checkout\" can emit seemingly nonsensical error\n    @@ Commit message\n         .git/MERGE_HEAD and other non-pseudoref looking refs at the top-level.\n     \n         Let's help the user in this case by doing a very loose check for\n    -    whether the ref name looks like a pseudoref such as \"HEAD\" (i.e. only\n    -    has upper case, dashes, underbars), and if not issue a warning:\n    +    whether the ref name looks like a special pseudoref such as\n    +    \"HEAD\" (i.e. only has upper case, dashes, underbars), and if not issue\n    +    a warning:\n     \n             $ git rev-parse master\n             warning: matched ref .git/master doesn't look like a pseudoref\n    @@ Commit message\n         instead of \"warning\" passes with this approach.\n     \n         Our own test suite makes use of a few refs in .git/ that aren't\n    -    produced by git itself, e.g. \"FOO\", \"TESTSYMREFTWO\" etc, external\n    +    produced by git itself, e.g. \"FOO\", \"TESTSYMREFTWO\" etc. External\n         tools probably rely on this as well, so I don't think it's viable to\n    -    e.g. have a whitelist of \"HEAD\", \"MERGE_HEAD\" etc. As an aside that\n    -    list is quite large, I counted 12 names used in the C code before I\n    -    abandoned that approach.\n    +    e.g. have a whitelist of them. That list is quite large just fr\n    +    git.git, I counted 12 names used in the C code before I abandoned that\n    +    approach.\n     \n         This approach of checking the case of e.g. \"master\" is not an issue on\n         case-insensitive filesystems, since we're not checking against the\n    @@ Commit message\n         without warning on that system. Now we'll warn about that.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n    +    Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Documentation/config/core.txt ##\n     @@ Documentation/config/core.txt: core.warnAmbiguousRefs::\n    @@ Documentation/config/core.txt: core.warnAmbiguousRefs::\n     ++\n     +These references are ignored by linkgit:for-each-ref[1], but resolved\n     +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be\n    -+confusing to have e.g. an errant `.git/master` being confused with\n    -+`.git/refs/heads/master`.\n    ++confusing to have e.g. an errant `.git/mybranch` being confused with\n    ++`.git/refs/heads/mybranch`.\n     +\n      core.compression::\n      \tAn integer -1..9, indicating a default compression level.\n    @@ refs.c: int expand_ref(struct repository *repo, const char *str, int len,\n      \t\t\t\t\t    this_result, &flag);\n      \t\tif (r) {\n     +\t\t\tif (warn_non_pseudo_refs &&\n    -+\t\t\t    !starts_with(fullref.buf, \"refs/\") &&\n    -+\t\t\t    !starts_with(r, \"refs/\") &&\n     +\t\t\t    !strchr(r, '/') &&\n     +\t\t\t    !is_any_pseudoref_syntax(r) &&\n    -+\t\t\t    !warned_on_non_pseudo_ref++) {\n    -+\t\t\t\t/*\n    -+\t\t\t\t * TRANSLATORS: The 1st argument is\n    -+\t\t\t\t * e.g. \"master\", and the 2nd can be\n    -+\t\t\t\t * e.g. \"master~10\".\n    -+\t\t\t\t */\n    -+\t\t\t\twarning(_(\"matched ref name .git/%s doesn't look like a pseudoref\"), r);\n    -+\t\t\t}\n    ++\t\t\t    !warned_on_non_pseudo_ref++)\n    ++\t\t\t\twarning(_(\".git/%s doesn't look like a pseudoref\"), r);\n      \t\t\tif (!refs_found++)\n      \t\t\t\t*ref = xstrdup(r);\n      \t\t\tif (!warn_ambiguous_refs)\n    @@ t/t1430-bad-ref-name.sh: test_expect_success 'branch -m can rename refs/heads/-d\n      \n     +test_expect_success 'warn on non-pseudoref syntax refs in .git/' '\n     +\ttest_when_finished \"\n    -+\t\trm -f .git/mybranch &&\n    -+\t\trm -rf .git/a-dir &&\n    -+\t\trm -rf .git/MY-BRANCH_NAME &&\n    -+\t\trm -rf .git/MY-branch_NAME\n    ++\t\trm -rf .git/mybranch \\\n    ++\t\t\t.git/a-dir \\\n    ++\t\t\t.git/MY-BRANCH_NAME \\\n    ++\t\t\t.git/MY-branch_NAME\n     +\t\" &&\n     +\n     +\t# Setup\n    -+\tgit rev-parse master >expect &&\n    ++\tgit rev-parse HEAD >expect &&\n    ++\tmkdir .git/a-dir &&\n     +\n     +\t# We ignore anything with slashes\n    -+\tmkdir .git/a-dir &&\n     +\tcp expect .git/a-dir/mybranch &&\n     +\tgit rev-parse a-dir/mybranch >hash 2>err &&\n     +\ttest_must_be_empty err &&\n    @@ t/t1430-bad-ref-name.sh: test_expect_success 'branch -m can rename refs/heads/-d\n     +\n     +\t# We do not ignore lower-case\n     +\tcp expect .git/mybranch &&\n    -+\tgit rev-parse mybranch >hash 2>err &&\n    ++\tenv GIT_TEST_GETTEXT_POISON=false \\\n    ++\t\tgit rev-parse mybranch >hash 2>err &&\n     +\ttest_cmp expect hash &&\n    -+\tGIT_TEST_GETTEXT_POISON=false grep \"like a pseudoref\" err &&\n    ++\tgrep \"like a pseudoref\" err &&\n     +\tgit -c core.warnNonPseudoRefs=false rev-parse mybranch >hash 2>err &&\n     +\ttest_cmp expect hash &&\n    -+\ttest_must_be_empty err &&\n    -+\trm .git/mybranch\n    ++\ttest_must_be_empty err\n     +'\n     +\n      test_done\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"412195","messageId":"CAPig+cTWoBaoQxNvbFhKNVSvYP_TFOqdfLsu5tOW32uO5TEoaQ@mail.gmail.com","threadId":"54808","inReplyTo":"20201214191700.16405-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-14T19:56:19Z","receivedAt":"2020-12-14T19:57:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 14, 2020 at 2:17 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> [...]\n> Our own test suite makes use of a few refs in .git/ that aren't\n> produced by git itself, e.g. \"FOO\", \"TESTSYMREFTWO\" etc. External\n> tools probably rely on this as well, so I don't think it's viable to\n> e.g. have a whitelist of them. That list is quite large just fr\n\nYou want a s/fr/from/ here or something?\n\n> git.git, I counted 12 names used in the C code before I abandoned that\n> approach.\n> [...]\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nIs the Modified-by: footer intentional?\n"},{"id":"412212","messageId":"xmqqczzcatyr.fsf@gitster.c.googlers.com","threadId":"54808","inReplyTo":"20201214191700.16405-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T22:44:28Z","receivedAt":"2020-12-14T22:45:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> The refs parsing machinery will first try to parse arbitrary\n> .git/<name> for a given <name>, before moving onto refs/<name>,\n> refs/tags/<name> etc. See \"ref_rev_parse_rules\" in refs.c. Things that\n> list references such as \"for-each-ref\" ignore these on the assumption\n> that they're pseudorefs such as \"HEAD\".\n>\n> Thus if you end up in a repository that contains e.g. .git/master the\n> likes of \"checkout\" can emit seemingly nonsensical error\n> messages. E.g. I happened to have a .git/master with a non-commit\n> SHA-1:\n>\n>     $ git checkout master\n>     fatal: Cannot switch branch to a non-commit 'master'\n\nOr, without even any funny files created in .git to make it\nmisbehave, \"git checkout config\" would already give you\n\n    $ git checkout config\n    error: pathspec 'config' did not match any file(s) known to git\n    $ git checkout config --\n    fatal: invalid reference: config\n\nYou were unlucky enough to have 40-hex in that garbage file.  How\ndid you end up with it, I wonder, but anyway, a move to make it\neasier to diagnose the situation is very welcome.\n\n> Let's help the user in this case by doing a very loose check for\n> whether the ref name looks like a special pseudoref such as\n> \"HEAD\" (i.e. only has upper case, dashes, underbars), and if not issue\n> a warning:\n>\n>     $ git rev-parse master\n>     warning: matched ref .git/master doesn't look like a pseudoref\n>     c87c83a2e9eb6d309913a0f59389f808024a58f9\n\nWith the problem this patch addressed solved, what should happen if\nyou did this?\n\n    $ git rev-parse refs/heads/master~4 >.git/master\n    $ git checkout master\n\nMy knee-jerk reaction to the question is that we should still give\nthe same warning, even though the \"checkout\" should successfully\ndetach the HEAD at the commit, to remind you that the name you used\ncame from may have been resolved to something you did not expect,\nand the value you used may not be what you wanted to use.\n\n> I think it's conservative enough to just turn this on by default, but\n> place it under a configurable option similar to the existing\n> core.warnAmbiguousRefs. Running the entire test suite with \"die\"\n> instead of \"warning\" passes with this approach.\n\nSounds sensible.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nHuh?\n\n> +core.warnNonPseudoRefs::\n> +\tIf true, Git will warn you if the `<ref>` you passed\n> +\tunexpectedly resolves to a top-level ref stored in\n> +\t`.git/<file>` but doesn't look like a pseudoref such as\n> +\t`HEAD`, `MERGE_HEAD` etc. True by default.\n\nI'd really prefer to see us to think, design and describe in terms\nof a more generic ref API, when we do not have to limit us to the\nbehaviour only with files backend.  Reading the file `.git/<ref>` on\nthe filesystem happens to be one way to resolve a ref to its value\n(i.e. reference to an object), but even with different ref backends,\nwe want 'master' that sits next to 'refs/heads/master' to trigger\nthis warning.  So\n\n\t... if the `<ref>` you passed refers to a <ref> outside the\n\t`refs/{heads,tags,...}/` hierarchy but does not look like\n\t...\n\nor something like that, perhaps.\n\n> ++\n> +These references are ignored by linkgit:for-each-ref[1], but resolved\n> +by linkgit:git-show[1], linkgit:git-rev-parse[1] etc. So it can be\n> +confusing to have e.g. an errant `.git/mybranch` being confused with\n> +`.git/refs/heads/mybranch`.\n\nHere, \".git/mybranch\" and \".git/refs/heads/mybranch\" are good as\nexamples.  I just want to avoid tying the main description to the\nfiles backend.\n\n> diff --git a/refs.c b/refs.c\n> index 3ec5dcba0be..634ab64cc9e 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -649,12 +649,19 @@ static int is_main_pseudoref_syntax(const char *refname)\n>  \t\tis_pseudoref_syntax(refname);\n>  }\n>  \n> +static int is_any_pseudoref_syntax(const char *refname)\n> +{\n> +\treturn is_main_pseudoref_syntax(refname) ||\n> +\t\tis_pseudoref_syntax(refname);\n> +}\n\nHmph, why is this needed, I wonder.\n\n\"git show main-worktree/master\" is specific enough to tell us that\nthe user did not mean \"git show main-worktree/refs/heads/master\",\nno?  If so, then wouldn't it be sufficient to check only with\nis_pseudoref_syntax() and nothing else?  I may probably missing\nsomething to make me convince is_main_pseudoref_syntax() matters\nhere.\n\n>  int expand_ref(struct repository *repo, const char *str, int len,\n>  \t       struct object_id *oid, char **ref)\n>  {\n>  \tconst char **p, *r;\n>  \tint refs_found = 0;\n>  \tstruct strbuf fullref = STRBUF_INIT;\n> +\tstatic int warned_on_non_pseudo_ref;\n>  \n>  \t*ref = NULL;\n>  \tfor (p = ref_rev_parse_rules; *p; p++) {\n> @@ -669,6 +676,11 @@ int expand_ref(struct repository *repo, const char *str, int len,\n>  \t\t\t\t\t    fullref.buf, RESOLVE_REF_READING,\n>  \t\t\t\t\t    this_result, &flag);\n>  \t\tif (r) {\n> +\t\t\tif (warn_non_pseudo_refs &&\n> +\t\t\t    !strchr(r, '/') &&\n> +\t\t\t    !is_any_pseudoref_syntax(r) &&\n> +\t\t\t    !warned_on_non_pseudo_ref++)\n> +\t\t\t\twarning(_(\".git/%s doesn't look like a pseudoref\"), r);\n\nI do not see much point in reporting only the first one.\n\nIsn't the conditional \"if (r)\" sufficiently limiting the ref only to\nthose that the user showed immediate interest in?  In other words,\neven if there were .git/foo and .git/bar, both of which happens to\ncontain something that looks like an object name, \"git show foo\"\nwould cause this code to warn only about .git/foo and .git/bar, no?\n\nIf there are more than one hits, why shouldn't we report all of\nthem?\n\nOr is this some performance thing (i.e. there only can be one hit,\nso repeated calls to strchr() and is_any_pseudoref_syntax() after\nseeing one hit would all be wasteful)?  If so, \"have we warned? if\nso skip the remainder of checking, as we won't warn\" should be the\nfirst in the &&-chain.\n\nPuzzled.\n\nThanks.\n"},{"id":"412308","messageId":"873606wtwo.fsf@evledraar.gmail.com","threadId":"54808","inReplyTo":"xmqqczzcatyr.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-15T23:07:03Z","receivedAt":"2020-12-15T23:08:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 14 2020, Junio C Hamano wrote:\n\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>\n> Huh?\n\nI think I was using that to test my %(trailers) patches & didn't\nnotice. Sorry.\n\nAs for the rest of the feedback thanks & I'll address it.\n\nI'm planning to re-roll this but given that we're in the rc window & the\ntime I have to re-roll this in the coming week(s) let's put this series\non hold/eject it (in case you were planning to put it into \"seen\") until\nwe start post-v2.30 work.\n"}]}