threads / patch / 22243

v3Add push --set-upstream

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

## tl;dr

11 messages between Jan 16, 2010 and Jan 16, 2010. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Ilari Liusvaara· Jan 16, 2010, 09:23 UTC · lore

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
Show changes to 5 files +135 −1

Documentation/git-push.txt, builtin-push.c, t/t5523-push-upstream.sh, transport.c, transport.h

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
Tay Ray Chuan· Jan 16, 2010, 12:35 UTC · re: Ilari Liusvaara · lore

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

Hi,

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

Show 6 quoted lines
> [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) {
		..
	}
Show 5 quoted lines
> [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:
Show 8 quoted lines
> @@ -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.)

Show changes to diff +3 −2
@@ -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
Ilari Liusvaara· Jan 16, 2010, 13:46 UTC · re: Tay Ray Chuan · lore

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

On Sat, Jan 16, 2010 at 08:35:57PM +0800, Tay Ray Chuan wrote:
Show 7 quoted lines
> 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.
Show 5 quoted lines
> > [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'.
Show 5 quoted lines
> 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
Tay Ray Chuan· Jan 16, 2010, 15:30 UTC · re: Ilari Liusvaara · lore

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

Hi,

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

Show 10 quoted lines
> > 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.
Show changes to diff +7 −2
@@ -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
Ilari Liusvaara· Jan 16, 2010, 15:56 UTC · re: Tay Ray Chuan · lore

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

On Sat, Jan 16, 2010 at 11:30:43PM +0800, Tay Ray Chuan wrote:
Show 20 quoted lines
> 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
Tay Ray Chuan· Jan 16, 2010, 16:13 UTC · re: Ilari Liusvaara · lore

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

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
Tay Ray Chuan· Jan 16, 2010, 18:12 UTC · re: Tay Ray Chuan · lore

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

Hi,
On Sun, Jan 17, 2010 at 12:13 AM, Tay Ray Chuan <rctay89@gmail.com> wrote:
Show 11 quoted lines
> 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
Jeff King· Jan 16, 2010, 18:28 UTC · re: Tay Ray Chuan · lore

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

On Sun, Jan 17, 2010 at 12:13:20AM +0800, Tay Ray Chuan wrote:
Show 9 quoted lines
> 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
Tay Ray Chuan· Jan 16, 2010, 18:39 UTC · re: Jeff King · lore

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

Hi,
On Sun, Jan 17, 2010 at 2:28 AM, Jeff King <peff@peff.net> wrote:
Show 16 quoted lines
> 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
Junio C Hamano· Jan 16, 2010, 16:47 UTC · re: Tay Ray Chuan · lore

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

Tay Ray Chuan <rctay89@gmail.com> writes:
Show 8 quoted lines
> +		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?

Tay Ray Chuan· Jan 16, 2010, 18:43 UTC · re: Ilari Liusvaara · lore

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

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
>

← back to recent threads