linux-kernel - Re: [RFC/PATCH] refs: tone down the dwimmery in refname

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874l5gezsn.fsf@evledraar.gmail.com>
Date:   Mon, 27 May 2019 16:29:28 +0200
From:   Ævar Arnfjörð Bjarmason <avarab@...il.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     git@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Junio C Hamano <gitster@...ox.com>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Radim Krčmář <rkrcmar@...hat.com>,
        KVM list <kvm@...r.kernel.org>,
        Michael Haggerty <mhagger@...m.mit.edu>
Subject: Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/*

On Mon, May 27 2019, Paolo Bonzini wrote:

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

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