Re: [PATCH v2 1/3] for-each-ref: let upstream/push optionally report the remote name

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2017, 02:07 UTC
Message-ID: <xmqqinfmo2sf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <964525df50e6d35847bcf57b0fe0949c99f0dbca.1507205895.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 12 quoted lines

> The implementation chooses *not* to DWIM the push remote if no explicit
> push remote was configured; The reason is that it is possible to DWIM this
> by using
>
> 	%(if)%(push:remotename)%(then)
> 		%(push:remotename)
> 	%(else)
> 		%(upstream:remotename)
> 	%(end)
>
> while it would be impossible to "un-DWIM" the information in case the
> caller is really only interested in explicit push remotes.

Good.

> Note: the dashless, non-CamelCased form `:remotename` follows the
> example of the `:trackshort` example.

Good. Come to think of it, aside from modifiers, the use of squashedwords like committerdate and objecttype has long been the norm in the for-each-ref atom names, so "remotename" is much more in line with other formats.

Show 23 quoted lines

> @@ -1354,16 +1371,20 @@ static void populate_value(struct ref_array_item *ref)
>  			if (refname)
>  				fill_remote_ref_details(atom, refname, branch, &v->s);
>  			continue;
> -		} else if (starts_with(name, "push")) {
> +		} else if (atom->u.remote_ref.push) {
>  			const char *branch_name;
>  			if (!skip_prefix(ref->refname, "refs/heads/",
>  					 &branch_name))
>  				continue;
>  			branch = branch_get(branch_name);
>  
> -			refname = branch_get_push(branch, NULL);
> -			if (!refname)
> -				continue;
> +			if (atom->u.remote_ref.push_remote)
> +				refname = NULL;
> +			else {
> +				refname = branch_get_push(branch, NULL);
> +				if (!refname)
> +					continue;
> +			}
>  			fill_remote_ref_details(atom, refname, branch, &v->s);

It still feels a bit strange to pass NULL refname to fill_remote_ref_details() function, but fixing it would require overhaul of this if/else if cascade, I think, so let's not worry too much about it at least for now.

Looks much better than the previous one.  Thanks.  Queued.