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

[Tay Ray Chuan <rctay89@gmail.com>]

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