[PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch

From: Ãvar ArnfjÃrÃ Bjarmason
Date: Fri Jun 21 2019 - 10:45:04 EST

Next message: Greg KH: "Re: PCI/AER sysfs files violate the rules of how sysfs works"
Previous message: Jason Gunthorpe: "Re: switch the remaining architectures to use generic GUP v3"
Next in thread: Junio C Hamano: "Re: [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx/
2. https://public-inbox.org/git/20190526225445.21618-1-avarab@xxxxxxxxx/

Signed-off-by: Ãvar ArnfjÃrÃ Bjarmason <avarab@xxxxxxxxx>
---

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

Next message: Greg KH: "Re: PCI/AER sysfs files violate the rules of how sysfs works"
Previous message: Jason Gunthorpe: "Re: switch the remaining architectures to use generic GUP v3"
Next in thread: Junio C Hamano: "Re: [PATCH] push: make "HEAD:tags/my-tag" consistently push to a branch"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]