[PATCH] fix "git push $there +HEAD"

[PATCH] fix "git push $there +HEAD"

[Junio C Hamano]

An earlier commit 47d996a (push: support pushing HEAD to real
branch name) added support for "git push $there HEAD" by
introducing a rewrite rule for the refspecs obtained from the
command line.  However, unlike the usual refspecs, it did not
allow prefixing with '+' to mean forcing the branch.

This refactors the rewriting rule into a separate function, and
teaches it to pay attention to a possible '+' prefix.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * If we were to do the "remote.*.push = HEAD" I mentioned
   earlier for defeating the default "matching" behaviour while
   pushing into shared repositories, we would need to apply the
   same rewriting rule for the refspecs obtained from remote at
   the beginning of do_push(), and that is what triggered this
   refactoring.

 builtin-push.c        |   40 +++++++++++++++++++++++++++++++---------
 t/t5516-fetch-push.sh |   21 +++++++++++++++++++++
 2 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/builtin-push.c b/builtin-push.c
index 9f727c0..0336ef8 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -28,6 +28,36 @@ static void add_refspec(const char *ref)
 	refspec_nr = nr;
 }
 
+static const char *replace_HEAD(const char *ref)
+{
+	static int checked_HEAD;
+	static char *plus_HEAD;
+	int force = 0;
+	const char *s = ref;
+
+	if (s[0] == '+') {
+		force = 1;
+		s++;
+	}
+	if (strcmp("HEAD", s))
+		return ref;
+	if (!checked_HEAD) {
+		unsigned char sha1[20];
+		size_t len;
+
+		checked_HEAD = 1;
+		s = resolve_ref(s, sha1, 1, NULL);
+		if (!s || prefixcmp(s, "refs/heads/"))
+			die("HEAD cannot be resolved to branch.");
+		len = strlen(s);
+		plus_HEAD = xmalloc(len + 2);
+		plus_HEAD[0] = '+';
+		strcpy(plus_HEAD + 1, s);
+	}
+
+	return plus_HEAD + !force;
+}
+
 static void set_refspecs(const char **refs, int nr)
 {
 	int i;
@@ -44,15 +74,7 @@ static void set_refspecs(const char **refs, int nr)
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
-		if (!strcmp("HEAD", ref)) {
-			unsigned char sha1_dummy[20];
-			ref = resolve_ref(ref, sha1_dummy, 1, NULL);
-			if (!ref)
-				die("HEAD cannot be resolved.");
-			if (prefixcmp(ref, "refs/heads/"))
-				die("HEAD cannot be resolved to branch.");
-			ref = xstrdup(ref + 11);
-		}
+		ref = replace_HEAD(ref);
 		add_refspec(ref);
 	}
 }
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 9d2dc33..3370d53 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -271,6 +271,27 @@ test_expect_success 'push with HEAD nonexisting at remote' '
 	check_push_result $the_commit heads/local
 '
 
+test_expect_success 'push with +HEAD' '
+
+	mk_test heads/master &&
+	git checkout master &&
+	git branch -D local &&
+	git checkout -b local &&
+	git push testrepo master local &&
+	check_push_result $the_commit heads/master &&
+	check_push_result $the_commit heads/local &&
+
+	# Without force rewinding should fail
+	git reset --hard HEAD^ &&
+	! git push testrepo HEAD &&
+	check_push_result $the_commit heads/local &&
+
+	# With force rewinding should succeed
+	git push testrepo +HEAD &&
+	check_push_result $the_first_commit heads/local
+
+'
+
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&

Re: [PATCH] fix "git push $there +HEAD"

[Johannes Schindelin]

Hi,

On Wed, 20 Feb 2008, Junio C Hamano wrote:

> An earlier commit 47d996a (push: support pushing HEAD to real branch 
> name) added support for "git push $there HEAD" by introducing a rewrite 
> rule for the refspecs obtained from the command line.  However, unlike 
> the usual refspecs, it did not allow prefixing with '+' to mean forcing 
> the branch.

Heh.  I realised that yesterday, and put it in my TODO list.  Thanks for 
doing it for me! ;-)

The patch looks obviously correct to me.

Ciao,
Dscho

Re: [PATCH] fix "git push $there +HEAD"

[Daniel Barkalow]

On Wed, 20 Feb 2008, Junio C Hamano wrote:

> An earlier commit 47d996a (push: support pushing HEAD to real
> branch name) added support for "git push $there HEAD" by
> introducing a rewrite rule for the refspecs obtained from the
> command line.  However, unlike the usual refspecs, it did not
> allow prefixing with '+' to mean forcing the branch.
> 
> This refactors the rewriting rule into a separate function, and
> teaches it to pay attention to a possible '+' prefix.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * If we were to do the "remote.*.push = HEAD" I mentioned
>    earlier for defeating the default "matching" behaviour while
>    pushing into shared repositories, we would need to apply the
>    same rewriting rule for the refspecs obtained from remote at
>    the beginning of do_push(), and that is what triggered this
>    refactoring.

If that's the ultimate goal, I think it would be better to have "<src>" 
treated as "<src>:<resolve of <src>>" instead of as "<src>:<src>" in 
match_refs(); and not do any special rewriting on the front end. How about 
this:

-----

commit a5de00049f4072e3ea54ff05bc265a4b48a5ce74
Author: Daniel Barkalow <barkalow@iabervon.org>
Date:   Wed Feb 20 12:54:05 2008 -0500

    Resolve value supplied for no-colon push refspecs
    
    When pushing a refspec like "HEAD", we used to treat it as
    "HEAD:HEAD", which didn't work without rewriting. Instead, we should
    resolve the ref. If it's a symref, further require it to point to a
    branch, to avoid doing anything especially unexpected. Also remove the
    rewriting previously added in builtin-push.
    
    Since the code for "HEAD" uses the regular refspec parsing, it
    automatically handles "+HEAD" without anything special.
    
    Passes the included test written by Junio Hamano.
    
    Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

diff --git a/builtin-push.c b/builtin-push.c
index c8cb63e..41df717 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -44,15 +44,6 @@ static void set_refspecs(const char **refs, int nr)
 			strcat(tag, refs[i]);
 			ref = tag;
 		}
-		if (!strcmp("HEAD", ref)) {
-			unsigned char sha1_dummy[20];
-			ref = resolve_ref(ref, sha1_dummy, 1, NULL);
-			if (!ref)
-				die("HEAD cannot be resolved.");
-			if (prefixcmp(ref, "refs/heads/"))
-				die("HEAD cannot be resolved to branch.");
-			ref = xstrdup(ref + 11);
-		}
 		add_refspec(ref);
 	}
 }
diff --git a/remote.c b/remote.c
index 6b56473..d2b19a8 100644
--- a/remote.c
+++ b/remote.c
@@ -643,9 +643,17 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		errs = 1;
 
 	if (!dst_value) {
+		unsigned char sha1[20];
+		int flag;
+
 		if (!matched_src)
 			return errs;
-		dst_value = matched_src->name;
+		dst_value = resolve_ref(matched_src->name, sha1, 1, &flag);
+		if (!dst_value ||
+		    ((flag & REF_ISSYMREF) && 
+		     prefixcmp(dst_value, "refs/heads/")))
+			die("%s cannot be resolved to branch.",
+			    matched_src->name);
 	}
 
 	switch (count_refspec_match(dst_value, dst, &matched_dst)) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 9d2dc33..3370d53 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -271,6 +271,27 @@ test_expect_success 'push with HEAD nonexisting at remote' '
 	check_push_result $the_commit heads/local
 '
 
+test_expect_success 'push with +HEAD' '
+
+	mk_test heads/master &&
+	git checkout master &&
+	git branch -D local &&
+	git checkout -b local &&
+	git push testrepo master local &&
+	check_push_result $the_commit heads/master &&
+	check_push_result $the_commit heads/local &&
+
+	# Without force rewinding should fail
+	git reset --hard HEAD^ &&
+	! git push testrepo HEAD &&
+	check_push_result $the_commit heads/local &&
+
+	# With force rewinding should succeed
+	git push testrepo +HEAD &&
+	check_push_result $the_first_commit heads/local
+
+'
+
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&

Re: [PATCH] fix "git push $there +HEAD"

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> ..., I think it would be better to have "<src>" 
> treated as "<src>:<resolve of <src>>" instead of as "<src>:<src>" in 
> match_refs(); and not do any special rewriting on the front end.

Wonderful.  That's hooking at the right layer.