| Message ID | 20201214191700.16405-3-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] refs: move is_pseudoref_syntax() earlier in the file | expand |
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?
Æ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.
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.
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
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(+)