From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git List <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs
Date: Thu, 10 Dec 2020 21:28:39 +0100 [thread overview]
Message-ID: <87sg8dwglk.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CAPig+cQpB3B+Ywc1aFd7QMrqVKy29VmNKJfzcFeSCYz=mwO0Hw@mail.gmail.com>
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
>> +'
next prev parent reply other threads:[~2020-12-10 20:31 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-10 12:53 [PATCH 1/2] refs: move is_pseudoref_syntax() earlier in the file Ævar Arnfjörð Bjarmason
2020-12-10 12:53 ` [PATCH 2/2] refs: warn on non-pseudoref looking .git/<file> refs Ævar Arnfjörð Bjarmason
2020-12-10 14:39 ` Eric Sunshine
2020-12-10 20:28 ` Ævar Arnfjörð Bjarmason [this message]
2020-12-10 20:47 ` Eric Sunshine
2020-12-10 22:17 ` Ævar Arnfjörð Bjarmason
2020-12-14 19:16 ` [PATCH v2 0/2] " Ævar Arnfjörð Bjarmason
2020-12-14 19:16 ` [PATCH v2 1/2] refs: move is_pseudoref_syntax() earlier in the file Ævar Arnfjörð Bjarmason
2020-12-14 19:17 ` [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs Ævar Arnfjörð Bjarmason
2020-12-14 19:56 ` Eric Sunshine
2020-12-14 22:44 ` Junio C Hamano
2020-12-15 23:07 ` Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sg8dwglk.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).