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

[Junio C Hamano <gitster@pobox.com>]

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