* Re: [GIT PULL] KVM changes for Linux 5.2-rc2 [not found] ` <2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com> @ 2019-05-26 20:49 ` Linus Torvalds 2019-05-26 22:54 ` [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2019-05-26 20:49 UTC (permalink / raw) To: Paolo Bonzini, Junio Hamano C Cc: Linux List Kernel Mailing, Radim Krčmář, KVM list, Git List Mailing 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. Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* 2019-05-26 20:49 ` [GIT PULL] KVM changes for Linux 5.2-rc2 Linus Torvalds @ 2019-05-26 22:54 ` Ævar Arnfjörð Bjarmason 2019-05-27 12:33 ` Paolo Bonzini 2019-06-21 14:44 ` [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 8+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-26 22:54 UTC (permalink / raw) To: git Cc: Linus Torvalds, Junio C Hamano, Paolo Bonzini, Linux List Kernel Mailing, Radim Krčmář, KVM list, Michael Haggerty, Ævar Arnfjörð Bjarmason 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(-) 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 && -- 2.21.0.1020.gf2820cf01a ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* 2019-05-26 22:54 ` [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* Ævar Arnfjörð Bjarmason @ 2019-05-27 12:33 ` Paolo Bonzini 2019-05-27 14:29 ` Ævar Arnfjörð Bjarmason 2019-06-21 14:44 ` [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2019-05-27 12:33 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, git Cc: Linus Torvalds, Junio C Hamano, Linux List Kernel Mailing, Radim Krčmář, KVM list, Michael Haggerty 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* 2019-05-27 12:33 ` Paolo Bonzini @ 2019-05-27 14:29 ` Ævar Arnfjörð Bjarmason 2019-05-27 15:39 ` Junio C Hamano 0 siblings, 1 reply; 8+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-27 14:29 UTC (permalink / raw) To: Paolo Bonzini Cc: git, Linus Torvalds, Junio C Hamano, Linux List Kernel Mailing, Radim Krčmář, KVM list, Michael Haggerty 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* 2019-05-27 14:29 ` Ævar Arnfjörð Bjarmason @ 2019-05-27 15:39 ` Junio C Hamano 2019-05-27 15:44 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Junio C Hamano @ 2019-05-27 15:39 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Paolo Bonzini, git, Linus Torvalds, Linux List Kernel Mailing, Radim Krčmář, KVM list, Michael Haggerty Æ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? ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* 2019-05-27 15:39 ` Junio C Hamano @ 2019-05-27 15:44 ` Paolo Bonzini 0 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2019-05-27 15:44 UTC (permalink / raw) To: Junio C Hamano, Ævar Arnfjörð Bjarmason Cc: git, Linus Torvalds, Linux List Kernel Mailing, Radim Krčmář, KVM list, Michael Haggerty 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch 2019-05-26 22:54 ` [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* Ævar Arnfjörð Bjarmason 2019-05-27 12:33 ` Paolo Bonzini @ 2019-06-21 14:44 ` Ævar Arnfjörð Bjarmason 2019-06-21 16:05 ` Junio C Hamano 1 sibling, 1 reply; 8+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-06-21 14:44 UTC (permalink / raw) To: git Cc: Linus Torvalds, Junio C Hamano, Paolo Bonzini, Linux List Kernel Mailing, Radim Krčmář, KVM list, Ævar Arnfjörð Bjarmason When a refspec like "HEAD:tags/my-tag" is pushed where "HEAD" is a branch, we'll push a *branch* that'll be located at "refs/heads/tags/my-tag". 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/my-tag" on the remote the count_refspec_match() logic will, as a result of calling refname_match(), match partially-qualified RHS of the refspec "refs/tags/my-tag", because it's in a loop where it tries to match "tags/my-tag" to "refs/tags/my-tag', then "refs/tags/tags/my-tag" 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 "refs/heads/tags/for-linus" branch in their "kvm" repository. But since they happened to have an existing "refs/tags/for-linus" reference we pushed there instead, and replaced an annotated tag with a lightweight tag. We do want a RHS ref like "master" to match "refs/heads/master", but it's confusing and dangerous that the DWYM behavior for matching partial RHS refspecs acts differently when the start of the RHS happens to be a second-level namespace under "refs/" namespace like "tags". Now we'll print out the following advice when this happens, and act differently as described therein: hint: The <dst> part of the refspec matched both of: hint: hint: 1. tags/my-tag -> refs/tags/my-tag hint: 2. tags/my-tag -> refs/heads/tags/my-tag hint: hint: Earlier versions of git would have picked (1) as the RHS starts hint: with a second-level ref prefix which could be fully-qualified by hint: adding 'refs/' in front of it. We now pick (2) which uses the prefix hint: inferred from the <src> part of the refspec. hint: hint: See the "<refspec>..." rules discussed in 'git help push'. An earlier version of this patch[2] used the much more heavy-handed approach of changing this logic in refname_match(). As shown from the tests that patch needed to modify that results in changes that are overzealous for fixing this push-specific issue. The right place to fix this is in match_explicit(). There we can see if we have both a DWYM match and a match based on the prefix of the LHS of the refspec, in those cases the match based on the LHS's ref prefix should win. 1. https://lore.kernel.org/lkml/2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com/ 2. https://public-inbox.org/git/20190526225445.21618-1-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Now that the 2.22.0 release is out I cleaned this up into a more sensible patch. Documentation/config/advice.txt | 7 +++++++ Documentation/git-push.txt | 13 +++++++++++++ advice.c | 2 ++ advice.h | 1 + remote.c | 23 ++++++++++++++++++++++- t/t5505-remote.sh | 18 ++++++++++++++++++ 6 files changed, 63 insertions(+), 1 deletion(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index ec4f6ae658..36cb3db63a 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -37,6 +37,13 @@ advice.*:: we can still suggest that the user push to either refs/heads/* or refs/tags/* based on the type of the source object. + pushPartialAmbigiousName:: + Shown when linkgit:git-push[1] is given a refspec + where the <src> in earlier versions of Git would have + matched a <dst> on the remote based on its existence + over appending a prefix based on the type of the + <src>. See the "<refspec>..." documentation in + linkgit:git-push[1] for details. statusHints:: Show directions on how to proceed from the current state in the output of linkgit:git-status[1], in diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 6a8a0d958b..5c46ef5e59 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -84,6 +84,19 @@ is ambiguous. * If <src> resolves to a ref starting with refs/heads/ or refs/tags/, then prepend that to <dst>. ++ +Versions of Git before 2.23.0 would override this rule and match +e.g. `HEAD:tags/mark` to either `refs/tags/mark` or `refs/tags/mark` +depending on, respectively, if `refs/tags/mark` existed or not on the +remote. ++ +We'll now consistently pick `refs/heads/tags/mark` based on this rule +and so that we're not so eager in guessing the <dst> on the remote +that we'll pick a different <dst> based on what refs exist there +already than we otherwise would have. This exception guards for cases +where the match would be different due to a subsequent +"refs/{tags,heads,remotes}/" matching rule". than a plain "refs/" +prefix match. * Other ambiguity resolutions might be added in the future, but for now any other cases will error out with an error indicating what we diff --git a/advice.c b/advice.c index ce5f374ecd..c9217834e2 100644 --- a/advice.c +++ b/advice.c @@ -10,6 +10,7 @@ int advice_push_already_exists = 1; int advice_push_fetch_first = 1; int advice_push_needs_force = 1; int advice_push_unqualified_ref_name = 1; +int advice_partial_ambiguous_ref_name = 1; int advice_status_hints = 1; int advice_status_u_option = 1; int advice_commit_before_merge = 1; @@ -66,6 +67,7 @@ static struct { { "pushFetchFirst", &advice_push_fetch_first }, { "pushNeedsForce", &advice_push_needs_force }, { "pushUnqualifiedRefName", &advice_push_unqualified_ref_name }, + { "pushPartialAmbigiousName", &advice_partial_ambiguous_ref_name }, { "statusHints", &advice_status_hints }, { "statusUoption", &advice_status_u_option }, { "commitBeforeMerge", &advice_commit_before_merge }, diff --git a/advice.h b/advice.h index e50f02cdfe..027ec396cf 100644 --- a/advice.h +++ b/advice.h @@ -10,6 +10,7 @@ extern int advice_push_already_exists; extern int advice_push_fetch_first; extern int advice_push_needs_force; extern int advice_push_unqualified_ref_name; +extern int advice_partial_ambiguous_ref_name; extern int advice_status_hints; extern int advice_status_u_option; extern int advice_commit_before_merge; diff --git a/remote.c b/remote.c index e50f7602ed..be226fac11 100644 --- a/remote.c +++ b/remote.c @@ -1066,7 +1066,7 @@ static int match_explicit(struct ref *src, struct ref *dst, struct ref ***dst_tail, struct refspec_item *rs) { - struct ref *matched_src, *matched_dst; + struct ref *matched_src, *matched_dst, *tmp; int allocated_src; const char *dst_value = rs->dst; @@ -1094,6 +1094,27 @@ static int match_explicit(struct ref *src, struct ref *dst, switch (count_refspec_match(dst_value, dst, &matched_dst)) { case 1: + if ((starts_with(dst_value, "tags/") || + starts_with(dst_value, "heads/") || + starts_with(dst_value, "remotes/")) && + (dst_guess = guess_ref(dst_value, matched_src))) { + tmp = make_linked_ref(dst_guess, dst_tail); + if (advice_partial_ambiguous_ref_name) + advise(_("The <dst> part of the refspec matched both of:\n" + "\n" + " 1. %1$s -> %2$s\n" + " 2. %1$s -> %3$s\n" + "\n" + "Earlier versions of git would have picked (1) as the RHS starts\n" + "with a second-level ref prefix which could be fully-qualified by\n" + "adding 'refs/' in front of it. We now pick (2) which uses the prefix\n" + "inferred from the <src> part of the refspec.\n" + "\n" + "See the \"<refspec>...\" rules discussed in 'git help push'.\n"), + dst_value, matched_dst->name, tmp->name); + matched_dst = tmp; + } + break; case 0: if (starts_with(dst_value, "refs/")) { diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 883b32efa0..4d54f90ae3 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1277,4 +1277,22 @@ 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 depending on if refs/tags/[AB] exists or not' ' + 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 2>advice && + test_i18ngrep "hint: The <dst> part of the refspec matched both of" advice && + git -C ../two rev-parse refs/heads/tags/my-tag >actual && + test_cmp expected actual + ) +' + test_done -- 2.22.0.455.g172b71a6c5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch 2019-06-21 14:44 ` [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch Ævar Arnfjörð Bjarmason @ 2019-06-21 16:05 ` Junio C Hamano 0 siblings, 0 replies; 8+ messages in thread From: Junio C Hamano @ 2019-06-21 16:05 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Linus Torvalds, Paolo Bonzini, Linux List Kernel Mailing, Radim Krčmář, KVM list Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This resulted in a case[1] where someone on LKML did: > > git push kvm +HEAD:tags/for-linus > > Which would have created a new "refs/heads/tags/for-linus" branch in > their "kvm" repository. But since they happened to have an existing > "refs/tags/for-linus" reference we pushed there instead, and replaced > an annotated tag with a lightweight tag. I do not think that is a problematic behaviour in the context of asking Linus to pull when every time a merge window opens. One would keep refs/tags/for-linus at the publishing site, and update it (forcing as necessary) before request-pull. If it went to a branch with confusing name tags/for-linus, that would be a disaster. > Now we'll print out the following advice when this happens, and act > differently as described therein: > > hint: The <dst> part of the refspec matched both of: > hint: > hint: 1. tags/my-tag -> refs/tags/my-tag > hint: 2. tags/my-tag -> refs/heads/tags/my-tag > hint: > hint: Earlier versions of git would have picked (1) as the RHS starts > hint: with a second-level ref prefix which could be fully-qualified by > hint: adding 'refs/' in front of it. We now pick (2) which uses the prefix > hint: inferred from the <src> part of the refspec. > hint: > hint: See the "<refspec>..." rules discussed in 'git help push'. "matched" in past tense means that your example scenario actually has such a confusing branch? Then I think the above is OK (or just silently updating the branch is also fine, I think). If there were no such branch currently, the above woudl be a serious regression, but as long as both exist, I think it is probably OK. From my quick scan of your new tests, I couldn't quite tell if that case (i.e. the a tag "my-tag" exists but a bbranch "tags/my-tag"does not exist at the receiving end when push happens, and the tag is updated without touching the branch nor giving extra warnings and hints) is covered, though. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-21 16:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1558864555-53503-1-git-send-email-pbonzini@redhat.com> [not found] ` <CAHk-=wi3YcO4JTpkeENETz3fqf3DeKc7-tvXwqPmVcq-pgKg5g@mail.gmail.com> [not found] ` <2d55fd2a-afbf-1b7c-ca82-8bffaa18e0d0@redhat.com> 2019-05-26 20:49 ` [GIT PULL] KVM changes for Linux 5.2-rc2 Linus Torvalds 2019-05-26 22:54 ` [RFC/PATCH] refs: tone down the dwimmery in refname_match() for {heads,tags,remotes}/* Ævar Arnfjörð Bjarmason 2019-05-27 12:33 ` Paolo Bonzini 2019-05-27 14:29 ` Ævar Arnfjörð Bjarmason 2019-05-27 15:39 ` Junio C Hamano 2019-05-27 15:44 ` Paolo Bonzini 2019-06-21 14:44 ` [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch Ævar Arnfjörð Bjarmason 2019-06-21 16:05 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).