[PATCH v3] Add push --set-upstream

[PATCH v3] Add push --set-upstream

[Ilari Liusvaara]

Frequent complaint is lack of easy way to set up upstream (tracking)
references for git pull to work as part of push command. So add switch
--set-upstream (-u) to do just that.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
Differences from v2:
- Only pretend to set up tracking in case of --dry-run.

 Documentation/git-push.txt |    9 +++++-
 builtin-push.c             |    1 +
 t/t5523-push-upstream.sh   |   69 ++++++++++++++++++++++++++++++++++++++++++++
 transport.c                |   56 +++++++++++++++++++++++++++++++++++
 transport.h                |    1 +
 5 files changed, 135 insertions(+), 1 deletions(-)
 create mode 100755 t/t5523-push-upstream.sh

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index e3eb1e8..2a5394b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
-	   [--repo=<repository>] [-f | --force] [-v | --verbose]
+	   [--repo=<repository>] [-f | --force] [-v | --verbose] [-u | --set-upstream]
 	   [<repository> <refspec>...]
 
 DESCRIPTION
@@ -122,6 +122,13 @@ nor in any Push line of the corresponding remotes file---see below).
 	the name "origin" is used. For this latter case, this option
 	can be used to override the name "origin". In other words,
 	the difference between these two commands
+
+-u::
+--set-upstream::
+	For every branch that is up to date or successfully pushed, add
+	upstream (tracking) reference, used by argument-less
+	linkgit:git-pull[1] and other commands. For more information,
+	see 'branch.<name>.merge' in linkgit:git-config[1].
 +
 --------------------------
 git push public         #1
diff --git a/builtin-push.c b/builtin-push.c
index 28a26e7..75ddaf4 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -218,6 +218,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
 		OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
 		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
+		OPT_BIT('u', "set-upstream", &flags, "Set upstream for git pull", TRANSPORT_PUSH_SET_UPSTREAM),
 		OPT_END()
 	};
 
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
new file mode 100755
index 0000000..00da707
--- /dev/null
+++ b/t/t5523-push-upstream.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='push with --set-upstream'
+. ./test-lib.sh
+
+test_expect_success 'setup bare parent' '
+	git init --bare parent &&
+	git remote add upstream parent
+'
+
+test_expect_success 'setup local commit' '
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+check_config() {
+	(echo $2; echo $3) >expect.$1
+	(git config branch.$1.remote
+	 git config branch.$1.merge) >actual.$1
+	test_cmp expect.$1 actual.$1
+}
+
+test_expect_success 'push -u master:master' '
+	git push -u upstream master:master &&
+	check_config master upstream refs/heads/master
+'
+
+test_expect_success 'push -u master:other' '
+	git push -u upstream master:other &&
+	check_config master upstream refs/heads/other
+'
+
+test_expect_success 'push -u --dry-run master:otherX' '
+	git push -u --dry-run upstream master:otherX &&
+	check_config master upstream refs/heads/other
+'
+
+test_expect_success 'push -u master2:master2' '
+	git branch master2 &&
+	git push -u upstream master2:master2 &&
+	check_config master2 upstream refs/heads/master2
+'
+
+test_expect_success 'push -u master2:other2' '
+	git push -u upstream master2:other2 &&
+	check_config master2 upstream refs/heads/other2
+'
+
+test_expect_success 'push -u :master2' '
+	git push -u upstream :master2 &&
+	check_config master2 upstream refs/heads/other2
+'
+
+test_expect_success 'push -u --all' '
+	git branch all1 &&
+	git branch all2 &&
+	git push -u --all &&
+	check_config all1 upstream refs/heads/all1 &&
+	check_config all2 upstream refs/heads/all2
+'
+
+test_expect_success 'push -u HEAD' '
+	git checkout -b headbranch &&
+	git push -u upstream HEAD &&
+	check_config headbranch upstream refs/heads/headbranch
+'
+
+test_done
diff --git a/transport.c b/transport.c
index b5332c0..12b3d69 100644
--- a/transport.c
+++ b/transport.c
@@ -8,6 +8,7 @@
 #include "bundle.h"
 #include "dir.h"
 #include "refs.h"
+#include "branch.h"
 
 /* rsync support */
 
@@ -135,6 +136,53 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 	}
 }
 
+static void set_upstreams(struct transport *trans, struct ref *refs,
+	int pretend)
+{
+	struct ref *i;
+	for (i = refs; i; i = i->next) {
+		const char *localname;
+		const char *tmp;
+		const char *remotename;
+		unsigned char sha[20];
+		int flag = 0;
+		/*
+		 * Check suitability for tracking. Must be successful /
+		 * alreay up-to-date ref create/modify (not delete).
+		 */
+		if (i->status != REF_STATUS_OK &&
+			i->status != REF_STATUS_UPTODATE)
+			continue;
+		if (!i->peer_ref)
+			continue;
+		if (!i->new_sha1 || is_null_sha1(i->new_sha1))
+			continue;
+
+		/* Chase symbolic refs (mainly for HEAD). */
+		localname = i->peer_ref->name;
+		remotename = i->name;
+		tmp = resolve_ref(localname, sha, 1, &flag);
+		if (tmp && flag & REF_ISSYMREF &&
+			!prefixcmp(tmp, "refs/heads/"))
+			localname = tmp;
+
+		/* Both source and destination must be local branches. */
+		if (!localname || prefixcmp(localname, "refs/heads/"))
+			continue;
+		if (!remotename || prefixcmp(remotename, "refs/heads/"))
+			continue;
+
+		if (!pretend)
+			install_branch_config(BRANCH_CONFIG_VERBOSE,
+				localname + 11, trans->remote->name,
+				remotename);
+		else
+			printf("Would set upstream '%s' -> '%s' of '%s'\n",
+				localname + 11, remotename + 11,
+				trans->remote->name);
+	}
+}
+
 static const char *rsync_url(const char *url)
 {
 	return prefixcmp(url, "rsync://") ? skip_prefix(url, "rsync:") : url;
@@ -974,6 +1022,10 @@ int transport_push(struct transport *transport,
 	verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
+		/* Maybe FIXME. But no important transport uses this case. */
+		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
+			die("This transport does not support using --set-upstream");
+
 		return transport->push(transport, refspec_nr, refspec, flags);
 	} else if (transport->push_refs) {
 		struct ref *remote_refs =
@@ -983,6 +1035,7 @@ int transport_push(struct transport *transport,
 		int verbose = flags & TRANSPORT_PUSH_VERBOSE;
 		int quiet = flags & TRANSPORT_PUSH_QUIET;
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
+		int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
 		int ret;
 
 		if (flags & TRANSPORT_PUSH_ALL)
@@ -1002,6 +1055,9 @@ int transport_push(struct transport *transport,
 					verbose | porcelain, porcelain,
 					nonfastforward);
 
+		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
+			set_upstreams(transport, remote_refs, pretend);
+
 		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
 			struct ref *ref;
 			for (ref = remote_refs; ref; ref = ref->next)
diff --git a/transport.h b/transport.h
index 97ba251..c4314dd 100644
--- a/transport.h
+++ b/transport.h
@@ -91,6 +91,7 @@ struct transport {
 #define TRANSPORT_PUSH_VERBOSE 16
 #define TRANSPORT_PUSH_PORCELAIN 32
 #define TRANSPORT_PUSH_QUIET 64
+#define TRANSPORT_PUSH_SET_UPSTREAM 128
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
-- 
1.6.6.102.gd6f8f.dirty

Re: [PATCH v3] Add push --set-upstream

[Tay Ray Chuan]

Hi,

On Sat, 16 Jan 2010 11:23:47 +0200
Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:

> [snip]
> @@ -218,6 +218,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>  		OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
>  		OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
>  		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
> +		OPT_BIT('u', "set-upstream", &flags, "Set upstream for git pull", TRANSPORT_PUSH_SET_UPSTREAM),

This line exceeds 80 characters.

Also, I think you should follow the case-style of the other option
descriptions and s/Set(?= upstream)/set/.

> [snip]
> +static void set_upstreams(struct transport *trans, struct ref *refs,

I would prefer if you could follow the naming convention and say
"transport" instead of truncating to "trans".

> +	int pretend)
> +{
> +	struct ref *i;
> +	for (i = refs; i; i = i->next) {

In most places, "ref" instead of "i" is used; ie.

	struct ref *ref;
	for (ref = refs; ref; ref = ref->next) {
		..
	}

> [snip]
> +		/*
> +		 * Check suitability for tracking. Must be successful /
> +		 * alreay up-to-date ref create/modify (not delete).
> +		 */

s/alreay/already/.

> [snip]
> +		/* Chase symbolic refs (mainly for HEAD). */

Did you mean "Change" here?

Regarding the checking of ref->status here:

> @@ -135,6 +136,53 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
>  	}
>  }
>  
> [snip]
> +		if (i->status != REF_STATUS_OK &&
> +			i->status != REF_STATUS_UPTODATE)
> +			continue;

Is it possible to delegate this to push_had_errors(remote_refs)
instead? We skip setting up upstream tracking when there are errors
from pushing, so we don't have to check ref->status anymore.

(Note: this would skip setting up upstream when ref->status is 'none',
in addition to 'ok' and 'uptodate', as you have it right now. I think
this should be safe.)

@@ -1002,8 +1055,9 @@ int transport_push(struct transport *transport,
					verbose | porcelain, porcelain,
					nonfastforward);

-		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
-			set_upstreams(transport, remote_refs, pretend);
+		if (!push_had_errors(remote_refs) && 
+			(flags & TRANSPORT_PUSH_SET_UPSTREAM))
+			set_upstreams(transport, remote_refs, pretend);

		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
			struct ref *ref;
			for (ref = remote_refs; ref; ref = ref->next)

(As a side note, if you apply this on top of 'tr/http-push-ref-status'
in 'pu', the result of push_had_errors() is saved in a variable
('err'), so you could just use it and not have to call push_had_errors
() again.)

-- 
Cheers,
Ray Chuan

Re: [PATCH v3] Add push --set-upstream

[Ilari Liusvaara]

On Sat, Jan 16, 2010 at 08:35:57PM +0800, Tay Ray Chuan wrote:
> Hi,
> 
> On Sat, 16 Jan 2010 11:23:47 +0200
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> > +		OPT_BIT('u', "set-upstream", &flags, "Set upstream for git pull", TRANSPORT_PUSH_SET_UPSTREAM),
> 
> This line exceeds 80 characters.

Broken into two (without breaking the message), it is still over 80.
 
> Also, I think you should follow the case-style of the other option
> descriptions and s/Set(?= upstream)/set/.

Fixed.

> > [snip]
> > +static void set_upstreams(struct transport *trans, struct ref *refs,
> 
> I would prefer if you could follow the naming convention and say
> "transport" instead of truncating to "trans".

Fixed.

> > +	struct ref *i;
> > +	for (i = refs; i; i = i->next) {
> 
> In most places, "ref" instead of "i" is used; ie.
 
Fixed.

> > +		 * alreay up-to-date ref create/modify (not delete).
> 
> s/alreay/already/.

Fixed.

> > +		/* Chase symbolic refs (mainly for HEAD). */
> 
> Did you mean "Change" here?

No. Changed to 'Follow'.

> Regarding the checking of ref->status here:
> 
> Is it possible to delegate this to push_had_errors(remote_refs)
> instead? We skip setting up upstream tracking when there are errors
> from pushing, so we don't have to check ref->status anymore.

No. As documetnation says, the update or no update is done on per-branch
basis.

<snip patch>
 
> (As a side note, if you apply this on top of 'tr/http-push-ref-status'
> in 'pu', the result of push_had_errors() is saved in a variable
> ('err'), so you could just use it and not have to call push_had_errors
> () again.)

See above.

I'll sit on v4 for some more time to wait for more comments.

-Ilari

Re: [PATCH v3] Add push --set-upstream

[Tay Ray Chuan]

Hi,

On Sat, 16 Jan 2010 15:46:57 +0200
Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:

> > Regarding the checking of ref->status here:
> > 
> > Is it possible to delegate this to push_had_errors(remote_refs)
> > instead? We skip setting up upstream tracking when there are errors
> > from pushing, so we don't have to check ref->status anymore.
> 
> No. As documetnation says, the update or no update is done on per-branch
> basis.
> 
> <snip patch>

I see. If that's the case, could you also allow setting up upstream
tracking when ref->status is 'none' and not consider it errorneous?

After all, push_had_errors() does not consider 'none' errorneous.

I think a switch block might be neater too.

@@ -149,9 +149,15 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
		 * Check suitability for tracking. Must be successful /
		 * already up-to-date ref create/modify (not delete).
		 */
-		if (ref->status != REF_STATUS_OK &&
-			ref->status != REF_STATUS_UPTODATE)
+		switch (ref->status) {
+		case REF_STATUS_NONE:
+		case REF_STATUS_UPTODATE:
+		case REF_STATUS_OK:
+			; /* no-op */
+		default:
			continue;
+		}
		if (!ref->peer_ref)
			continue;
		if (!ref->new_sha1 || is_null_sha1(ref->new_sha1))


-- 
Cheers,
Ray Chuan

Re: [PATCH v3] Add push --set-upstream

[Ilari Liusvaara]

On Sat, Jan 16, 2010 at 11:30:43PM +0800, Tay Ray Chuan wrote:
> Hi,
> 
> On Sat, 16 Jan 2010 15:46:57 +0200
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> 
> > > Regarding the checking of ref->status here:
> > > 
> > > Is it possible to delegate this to push_had_errors(remote_refs)
> > > instead? We skip setting up upstream tracking when there are errors
> > > from pushing, so we don't have to check ref->status anymore.
> > 
> > No. As documetnation says, the update or no update is done on per-branch
> > basis.
> > 
> > <snip patch>
> 
> I see. If that's the case, could you also allow setting up upstream
> tracking when ref->status is 'none' and not consider it errorneous?
> 
> After all, push_had_errors() does not consider 'none' errorneous.

Hmm... In what conditions ref->status is 'none' after push operation
has completed?

-Ilari

Re: [PATCH v3] Add push --set-upstream

[Tay Ray Chuan]

Hi,

On Sat, Jan 16, 2010 at 11:56 PM, Ilari Liusvaara
<ilari.liusvaara@elisanet.fi> wrote:
> Hmm... In what conditions ref->status is 'none' after push operation
> has completed?

when a match between a local and remote ref is not found.

For example, the local ref 'master' will match the remote ref
'master', but not 'retsam' ('master' spelled in reverse).

-- 
Cheers,
Ray Chuan

Re: [PATCH v3] Add push --set-upstream

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> +		switch (ref->status) {
> +		case REF_STATUS_NONE:
> +		case REF_STATUS_UPTODATE:
> +		case REF_STATUS_OK:
> +			; /* no-op */
> +		default:
> 			continue;
> +		}

Could somebody document what these values (and what "struct ref" has in
general) mean in the transport API documentation?

Re: [PATCH v3] Add push --set-upstream

[Tay Ray Chuan]

Hi,

On Sun, Jan 17, 2010 at 12:13 AM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> Hi,
>
> On Sat, Jan 16, 2010 at 11:56 PM, Ilari Liusvaara
> <ilari.liusvaara@elisanet.fi> wrote:
>> Hmm... In what conditions ref->status is 'none' after push operation
>> has completed?
>
> when a match between a local and remote ref is not found.
>
> For example, the local ref 'master' will match the remote ref
> 'master', but not 'retsam' ('master' spelled in reverse).

I'd like to add that in this case ('none'), no data is pushed to the
remote repo.

This is true for 'uptodate' too, and shouldn't be taken as an error.

-- 
Cheers,
Ray Chuan

Re: [PATCH v3] Add push --set-upstream

[Jeff King]

On Sun, Jan 17, 2010 at 12:13:20AM +0800, Tay Ray Chuan wrote:

> On Sat, Jan 16, 2010 at 11:56 PM, Ilari Liusvaara
> <ilari.liusvaara@elisanet.fi> wrote:
> > Hmm... In what conditions ref->status is 'none' after push operation
> > has completed?
> 
> when a match between a local and remote ref is not found.
> 
> For example, the local ref 'master' will match the remote ref
> 'master', but not 'retsam' ('master' spelled in reverse).

How would it make any sense to set up a tracking branch, then? Not only
is it not desired (if I said "git push -u foo:bar", I don't expect it to
do _anything_ with some other local branch besides foo), but it wouldn't
work, since the peer_ref pointer would be NULL.

-Peff

Re: [PATCH v3] Add push --set-upstream

[Tay Ray Chuan]

Hi,

On Sun, Jan 17, 2010 at 2:28 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Jan 17, 2010 at 12:13:20AM +0800, Tay Ray Chuan wrote:
>
>> On Sat, Jan 16, 2010 at 11:56 PM, Ilari Liusvaara
>> <ilari.liusvaara@elisanet.fi> wrote:
>> > Hmm... In what conditions ref->status is 'none' after push operation
>> > has completed?
>>
>> when a match between a local and remote ref is not found.
>>
>> For example, the local ref 'master' will match the remote ref
>> 'master', but not 'retsam' ('master' spelled in reverse).
>
> How would it make any sense to set up a tracking branch, then? Not only
> is it not desired (if I said "git push -u foo:bar", I don't expect it to
> do _anything_ with some other local branch besides foo), but it wouldn't
> work, since the peer_ref pointer would be NULL.

Noted.

Ilari, please ignore my thought on 'none' then.

-- 
Cheers,
Ray Chuan

Re: [PATCH v3] Add push --set-upstream

[Tay Ray Chuan]

Hi,

yet again, this patch has gone through another iteration, so I'm
adding people from the 'git push --track' thread here.

-- 
Cheers,
Ray Chuan

On Sat, Jan 16, 2010 at 9:46 PM, Ilari Liusvaara
<ilari.liusvaara@elisanet.fi> wrote:
> On Sat, Jan 16, 2010 at 08:35:57PM +0800, Tay Ray Chuan wrote:
>> Hi,
>>
>> On Sat, 16 Jan 2010 11:23:47 +0200
>> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
>> > +           OPT_BIT('u', "set-upstream", &flags, "Set upstream for git pull", TRANSPORT_PUSH_SET_UPSTREAM),
>>
>> This line exceeds 80 characters.
>
> Broken into two (without breaking the message), it is still over 80.
>
>> Also, I think you should follow the case-style of the other option
>> descriptions and s/Set(?= upstream)/set/.
>
> Fixed.
>
>> > [snip]
>> > +static void set_upstreams(struct transport *trans, struct ref *refs,
>>
>> I would prefer if you could follow the naming convention and say
>> "transport" instead of truncating to "trans".
>
> Fixed.
>
>> > +   struct ref *i;
>> > +   for (i = refs; i; i = i->next) {
>>
>> In most places, "ref" instead of "i" is used; ie.
>
> Fixed.
>
>> > +            * alreay up-to-date ref create/modify (not delete).
>>
>> s/alreay/already/.
>
> Fixed.
>
>> > +           /* Chase symbolic refs (mainly for HEAD). */
>>
>> Did you mean "Change" here?
>
> No. Changed to 'Follow'.
>
>> Regarding the checking of ref->status here:
>>
>> Is it possible to delegate this to push_had_errors(remote_refs)
>> instead? We skip setting up upstream tracking when there are errors
>> from pushing, so we don't have to check ref->status anymore.
>
> No. As documetnation says, the update or no update is done on per-branch
> basis.
>
> <snip patch>
>
>> (As a side note, if you apply this on top of 'tr/http-push-ref-status'
>> in 'pu', the result of push_had_errors() is saved in a variable
>> ('err'), so you could just use it and not have to call push_had_errors
>> () again.)
>
> See above.
>
> I'll sit on v4 for some more time to wait for more comments.
>
> -Ilari
>