Re: [PATCH v2 2/2] refs: warn on non-pseudoref looking .git/<file> refs
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 13 quoted lines
> 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: configYou 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.
Show 8 quoted lines
> 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 masterMy 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.
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Modified-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Show 5 quoted lines
> +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.
Show 5 quoted lines
> ++
> +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.
Show 13 quoted lines
> 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.
Show 19 quoted lines
> 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.
Ævar Arnfjörð Bjarmason
