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: <xmqqef4jewj6.fsf@gitster-ct.c.googlers.com>
Date:   Mon, 27 May 2019 08:39:57 -0700
From:   Junio C Hamano <gitster@...ox.com>
To:     Ævar Arnfjörð Bjarmason <avarab@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, git@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        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}/*

Ævar Arnfjörð Bjarmason <avarab@...il.com> writes:

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

Yes.

(I am still not up to speed, so pardon me if I sound nonsense)

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

OK, so you are saying "if the source is unique, try to qualify the
destination to the same hierarchy (i.e. the previous paragraph). If
the source is not a ref (this paragraph), try to find a unique match
with the destination to determine where it should go".  I think that
makes sense.

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

OK.

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

Are you saying that HEAD is locally dereferenced to a branch name
(if you are not detached when pushing), and "if the source is unique
ref" rule is applied first?  That is not how I recall we designed
this dwimmery.  As we know there is no refs/heads/HEAD, it should be
like pushing HEAD^0:tags/for-linus (i.e. it should behave the same
way as pushing "<some random SHA-1>:tags/for-linus"), without "where
is the source?  let's qualify the destination the same way" rule
kicking in.  And because the repeated "Linus, please pull from that
usual tag for this cycle" request is a norm, "does the destination
uniquely exist at the receiving end" should kick in.  IOW, I think
that is quite a deliberate behaviour that is desirable, or atleast
was considered to be desirable when the feature was designed.

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

Hmm (yes, I realize I am not commenting on what Ævar wrote)...

>>   # create remote lightweight tag and prepare a pull request
>>   git push ../b HEAD:refs/tags/tag1
>>   git request-pull HEAD^ ../b tags/tag1

I do not think lightweight vs annotated should be the issue.  The
tag that the requestor asks to be pulled (from repository ../b)
should be what the requestor has locally when writing the request
(in repository .).  Even if both tags at remote and local are
annotated, we should still warn if they are different objects, no?

Do we run ls-remote or something (or consult remote-trakcing branch)
to see if that is the case in request-pull?
?