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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 10, 2020, 14:39 UTC
Message-ID: <CAPig+cQpB3B+Ywc1aFd7QMrqVKy29VmNKJfzcFeSCYz=mwO0Hw@mail.gmail.com>
In-Reply-To: <20201210125321.19456-2-avarab@gmail.com>

On Thu, Dec 10, 2020 at 7:55 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 19 quoted lines

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

Show 15 quoted lines

> 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

Show 9 quoted lines

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

Show 5 quoted lines

> +       # 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?

Show 5 quoted lines

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