| Message ID | 20190526225445.21618-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* | expand |
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). 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 Paolo
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.
Ævar Arnfjörð Bjarmason <avarab@gmail.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? ?
On 27/05/19 17:39, Junio C Hamano wrote: > 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? Right, lightweight vs annotated then is the obvious special case where one of the two is a commit and the other is a tag, hence they ought not to have the same SHA1. I'll take a look. Paolo
diff --git a/refs.c b/refs.c index 92d1f6dbdd..729b921328 100644 --- a/refs.c +++ b/refs.c @@ -514,9 +514,15 @@ int refname_match(const char *abbrev_name, const char *full_name) const int abbrev_name_len = strlen(abbrev_name); const int num_rules = NUM_REV_PARSE_RULES; - for (p = ref_rev_parse_rules; *p; p++) + for (p = ref_rev_parse_rules; *p; p++) { + if (!strcmp(*p, "refs/%.*s") && + (starts_with(abbrev_name, "tags/") || + starts_with(abbrev_name, "heads/") || + starts_with(abbrev_name, "remotes/"))) + continue; if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) return &ref_rev_parse_rules[num_rules] - p; + } return 0; } diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh index fca001eb9b..0265871cf4 100755 --- a/t/t5150-request-pull.sh +++ b/t/t5150-request-pull.sh @@ -212,7 +212,7 @@ test_expect_success 'pull request format' ' cd local && git checkout initial && git merge --ff-only master && - git push origin tags/full && + git push origin full:refs/tags/full && git request-pull initial "$downstream_url" tags/full >../request ) && <request sed -nf fuzz.sed >request.fuzzy && diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 883b32efa0..52507b9e50 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1277,4 +1277,21 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and ) ' +test_expect_success 'HEAD:tags/A and HEAD:tags/B should not be different one of refs/tags/[AB] exists' ' + git clone "file://$PWD/two" tags-match && + ( + cd tags-match && + test_commit A && + git rev-parse HEAD >expected && + + git push origin HEAD:tags/my-not-a-tag && + git -C ../two rev-parse refs/heads/tags/my-not-a-tag >actual && + test_cmp expected actual && + + git push origin HEAD:tags/my-tag && + git -C ../two rev-parse refs/heads/tags/my-tag >actual && + test_cmp expected actual + ) +' + test_done diff --git a/t/t9101-git-svn-props.sh b/t/t9101-git-svn-props.sh index c26c4b0927..f9e43f4e97 100755 --- a/t/t9101-git-svn-props.sh +++ b/t/t9101-git-svn-props.sh @@ -73,11 +73,11 @@ test_expect_success 'fetch revisions from svn' 'git svn fetch' name='test svn:keywords ignoring' test_expect_success "$name" \ - 'git checkout -b mybranch remotes/git-svn && + 'git checkout -b mybranch refs/remotes/git-svn && echo Hi again >> kw.c && git commit -a -m "test keywords ignoring" && - git svn set-tree remotes/git-svn..mybranch && - git pull . remotes/git-svn' + git svn set-tree refs/remotes/git-svn..mybranch && + git pull . refs/remotes/git-svn' expect='/* $Id$ */' got="$(sed -ne 2p kw.c)" @@ -95,7 +95,7 @@ test_expect_success "propset CR on crlf files" ' test_expect_success 'fetch and pull latest from svn and checkout a new wc' \ 'git svn fetch && - git pull . remotes/git-svn && + git pull . refs/remotes/git-svn && svn_cmd co "$svnrepo" new_wc' for i in crlf ne_crlf lf ne_lf cr ne_cr empty_cr empty_lf empty empty_crlf @@ -117,7 +117,7 @@ cd test_wc svn_cmd commit -m "propset CRLF on cr files"' cd .. test_expect_success 'fetch and pull latest from svn' \ - 'git svn fetch && git pull . remotes/git-svn' + 'git svn fetch && git pull . refs/remotes/git-svn' b_cr="$(git hash-object cr)" b_ne_cr="$(git hash-object ne_cr)" @@ -168,7 +168,7 @@ cat >create-ignore-index.expect <<\EOF EOF test_expect_success 'test create-ignore' " - git svn fetch && git pull . remotes/git-svn && + git svn fetch && git pull . refs/remotes/git-svn && git svn create-ignore && cmp ./.gitignore create-ignore.expect && cmp ./deeply/.gitignore create-ignore.expect &&
When a refspec like HEAD:tags/x is pushed where HEAD is a branch, we'll push a *branch* that'll be located at "refs/heads/tags/x". This is part of the rather straightforward rules I documented in 2219c09e23 ("push doc: document the DWYM behavior pushing to unqualified <dst>", 2018-11-13). However, if there exists a refs/tags/x on the remote the count_refspec_match() logic will, as a result of calling refname_match() match the detected branch type of the LHS of the refspec to refs/tags/x, because it's in a loop where it tries to match "tags/x" to "refs/tags/X', then "refs/tags/tags/x" etc. 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. Let's tone this down a bit and not match the more general expansions if they'd overlap with later expansions. This patch is a hack, and should not be applied. We probably want to fix this for "push", but we use refname_match() all over the place. We probably want to start by undoing part of 54457fe509 ("refname_match(): always use the rules in ref_rev_parse_rules", 2014-01-14) and having special rules just for push. Furthermore ref_rev_parse_rules is used elsewhere, should we be doing this in other places? I think not if we undo most of 54457fe509 and can just have a custom matcher just for count_refspec_match(). That one shouldn't need any sort of magic, because elsewhere in the remote push DWYM code we try to add implicit refs/{tags,heads}/ prefixes. As the t/t5150-request-pull.sh change shows this results in a failing test where a local "full" branch is being pushed to a remote "refs/tags/full". So maybe this is something LKML people actually want for some reason. 1. https://lore.kernel.org/lkml/2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- On Sun, May 26 2019, Linus Torvalds wrote: > On Sun, May 26, 2019 at 10:53 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> The interesting thing is that not only git will treat lightweight tags >> like, well, tags: > > Yeah, that's very much by design - lightweight tags are very > comvenient for local temporary stuff where you don't want signing etc > (think automated test infrastructure, or just local reminders). > >> In addition, because I _locally_ had a tag object that >> pointed to the same commit and had the same name, git-request-pull >> included my local tag's message in its output! I wonder if this could >> be considered a bug. > > Yeah, I think git request-pull should at least *warn* about the tag > not being the same object locally as in the remote you're asking me to > pull. > > Are you sure you didn't get a warning, and just missed it? But adding > Junio and the Git list just as a possible heads-up for this in case > git request-pull really only compares the object the tag points to, > rather than the SHA1 of the tag itself. This behavior looks like a bug to me. This RFC-quality patch is an initial stab at fixing it, and is all I had time for today. refs.c | 8 +++++++- t/t5150-request-pull.sh | 2 +- t/t5505-remote.sh | 17 +++++++++++++++++ t/t9101-git-svn-props.sh | 12 ++++++------ 4 files changed, 31 insertions(+), 8 deletions(-)