Re: [PATCH] git push --track

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2010, 05:47 UTC
Message-ID: <7vvdf33onp.fsf@alter.siamese.dyndns.org>
In-Reply-To: <op.u6haiiiog402ra@nb-04>

"Rudolf Polzer" <divVerent@alientrap.org> writes:

Show 8 quoted lines

> On Wed, 13 Jan 2010 16:43:10 +0100, Ilari Liusvaara
> <ilari.liusvaara@elisanet.fi> wrote:
>
>> - Some lines look way too long (~160 chars, should be max 80 unles
>> it would linebreak error message).
>
> Yes, also I got told that I used the wrong braces style... well, fixed
> that.

You didn't, although you tried to "hide" one level, which is even worse.

If you see overlong lines in patch text, it often is that the added codepath is too deeply nested, and it often becomes much easier to understand if you split it into a separate smaller helper function.

For example, if you have

	if (A) {
        	if (B) {
                	do something #1
                        if (C) {
                        	do something #2
                                while (D) {
                                	if (E) {
                                        	do something #3
					}
				}
			}
		}
	}

it often is much easier to read if you did:

	if (A)
		helper(...)

and wrote a helper that is

	helper()
        {
        	if (!B)
                	return
		do something #1
                if (!C)
                	return
		do something #2
		while (D) {
                	if (!E)
                        	continue
			do something #3
		}
	}

>> - Should the tracking be set up even if only part of ref update suceeded
>> (for those that succeeded), not requiring all to succeed?

I think giving configuration to the ones that succeeded, while not doing so for the ones that failed, would be the best.

>> - Is --track the best name for this?

Most probably not. "git branch --track" was already a mistake, whose damage can be seen in the first message in this thread. I originally read "this converts a local branch to a tracking branch", and went "Huh??? --- Is this patch running 'mv refs/heads/frotz refs/remotes/origin/frotz'? What's fun about it???"

Show 6 quoted lines

> @@ -115,6 +116,36 @@ static int push_with_options(struct transport
> *transport, int flags)
>  		fprintf(stderr, "Pushing to %s\n", transport->url);
>  	err = transport_push(transport, refspec_nr, refspec, flags,
>  			     &nonfastforward);
> +	if (err == 0 && flags & TRANSPORT_PUSH_TRACK) {

Style:

 - Have SP between syntactic keyword and open parenthesis.

 - Never place an opening brace, except the one that begins a function
   body, on its own line;

Also the overlong line is merely a symptom that you are putting too much stuff in this function. The whole addition should probably be a helper function.

Show 8 quoted lines

> @@ -115,6 +116,33 @@ static int push_with_options(struct transport *transport, int flags)
>  		fprintf(stderr, "Pushing to %s\n", transport->url);
>  	err = transport_push(transport, refspec_nr, refspec, flags,
>  			     &nonfastforward);
> +	if (err == 0 && flags & TRANSPORT_PUSH_TRACK)
> +	{
> +		struct ref *remote_refs =
> +			transport->get_refs_list(transport, 1);

You have already pushed by calling transport_push() before you got "err" back. Do you need to make a second, separate call to ls-remote here and if so why?

I have a feeling that it is more appropriate to have the additional code in transport_push(), which gets ls-remote information, runs match_refs() and finally calls transport->push_refs(). I think the extra branch configuration would fit better inside the if block immediately after all that happens, i.e.

	if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
		struct ref *ref;
		for (ref = remote_refs; ref; ref = ref->next)
			update_tracking_ref(transport->remote, ref, verbose);
+		if (flags & TRANSPORT_PUSH_RECONFIGURE_FORK)
+			configure_forked_branch(...);
	}

in transport.c

> +		if(!(flags & TRANSPORT_PUSH_DRY_RUN))
> +		if(!match_refs(local_refs, &remote_refs, refspec_nr, refspec, match_flags))

Yuck; hiding the fact that you have an over-nested logic is not a way to fix it.