Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 27, 2019, 14:29 UTC
Message-ID: <874l5gezsn.fsf@evledraar.gmail.com>
In-Reply-To: <5c9ce55c-2c3a-fce0-d6e3-dfe5f8fc9b01@redhat.com>

On Mon, May 27 2019, Paolo Bonzini wrote:

Show 13 quoted lines

> On 27/05/19 00:54, Ævar Arnfjörð Bjarmason wrote:
>> This resulted in a case[1] where someone on LKML did:
>>
>>     git push kvm +HEAD:tags/for-linus
>>
>> Which would have created a new "tags/for-linus" branch in their "kvm"
>> repository, except because they happened to have an existing
>> "refs/tags/for-linus" reference we pushed there instead, and replaced
>> an annotated tag with a lightweight tag.
>
> Actually, I would not be surprised even if "git push foo
> someref:tags/foo" _always_ created a lightweight tag (i.e. push to
> refs/tags/foo).

That's not the intention (I think), and not what we document.

It mostly (and I believe always should) works by looking at whether "someref" is a named ref, and e.g. looking at whether it's "master". We then see that it lives in "refs/heads/master" locally, and thus correspondingly add a "refs/heads/" to your <dst> "tags/foo", making it "refs/heads/tags/foo".

*Or* we take e.g. <some random SHA-1>:master, the <some random...> is ambiguous, but we see that "master" unambiguously refers to "refs/heads/master" on the remote (so e.g. a refs/tags/master doesn't exist). If you had both refs/{heads,tags}/master refs on the remote we'd emit:

    error: dst refspec master matches more than one

(We should improve that error to note what conflicted, #leftoverbits)

So your HEAD:tags/for-linus resulted in pushing a HEAD that referred to some refs/heads/* to refs/tags/for-linus. I believe that's an unintendedem ergent effect in how we try to apply these two rules. We should apply one, not both in combination.

And as an aside none of these rules have to do with whether the <src> is a lightweight or annotated tag, and both types live in the refs/tags/* namespace.

Show 23 quoted lines

> In my opinion, the bug is that "git request-pull" should warn if the tag
> is lightweight remotely but not locally, and possibly even vice versa.
> Here is a simple testcase:
>
>   # setup "local" repo
>   mkdir -p testdir/a
>   cd testdir/a
>   git init
>   echo a > test
>   git add test
>   git commit -minitial
>
>   # setup "remote" repo
>   git clone --bare . ../b
>
>   # setup "local" tag
>   echo b >> test
>   git commit -msecond test
>   git tag -mtag tag1
>
>   # create remote lightweight tag and prepare a pull request
>   git push ../b HEAD:refs/tags/tag1
>   git request-pull HEAD^ ../b tags/tag1

Yeah, maybe. I don't use git-request-pull. So maybe this is a simple mitigation for that tool since you supply a <remote> to it already.

I was more interested and surprised by HEAD being implicitly resolved to refs/tags/* in a way that would be *different* than if you didn't have an existing tag there, but of course if we errored on that you might have just done "+HEAD:refs/tags/for-linus" and ended up with the same thing.

As an aside, in *general* tags, unlike branches, don't have "remote tracking". That's something we'd eventually want, but we're nowhere near the refstore and porcelain supporting that.

Thus such a check is hard to support in general, we'd always need a remote name and a network roundtrip. Otherwise we couldn't do anything sensible if you have 10 remotes of fellow LKML developers, all of whom have a "for-linus" tag, which I'm assuming is a common use-case.

But since git-request-pull gets the remote it can (and does) check on that remote, but seems to satisfied to see that the ref exists somewhere on that remote.