Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 10, 2020, 20:28 UTC
- Message-ID
- <87sg8dwglk.fsf@evledraar.gmail.com>
- In-Reply-To
- <CAPig+cQpB3B+Ywc1aFd7QMrqVKy29VmNKJfzcFeSCYz=mwO0Hw@mail.gmail.com>
On Thu, Dec 10 2020, Eric Sunshine wrote:
Show 24 quoted lines
> 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.
Show 22 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 pseudorefYeah that's much better, the TRANSLATORS comment is incorrect, rebased out an earlier version where it took two, didn't notice...
Show 13 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 ...
Will fix.
Show 7 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?
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.
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 >> +'