Re: [PATCH 2/3] for-each-ref: let upstream/push optionally report merge name.

[Junio C Hamano <gitster@pobox.com>]

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

> From: J Wyman <jwyman@microsoft.com>
>
> There are times when scripts want to know not only the name of the
> push branch on the remote, but also the name of the branch as known
> by the remote repository.
>
> A useful example of this is with the push command where
> `branch.<name>.merge` is useful as the <to> value. Example:
>
> 	$ git push <remote> <from>:<to>
>
> This patch offers the new suffix :merge for the push atom, allowing to
> show exactly that. Example:
>
> 	$ cat .git/config
> 	...
> 	[remote "origin"]
> 		url = https://where.do.we.come/from
> 		fetch = refs/heads/*:refs/remote/origin/*
> 	[branch "master"]
> 		remote = origin
> 		merge = refs/heads/master
> 	[branch "develop/with/topics"]
> 		remote = origin
> 		merge = refs/heads/develop/with/topics
> 	...
>
> 	$ git for-each-ref \
> 		--format='%(push) %(push:remote-ref)' \
> 		refs/heads
> 	refs/remotes/origin/master refs/heads/master
> 	refs/remotes/origin/develop/with/topics refs/heads/develop/with/topics

Ah, now it is clear that we _need_ to figure out how to spell
"multi-word" modifier to the format specifiers, as "push:remote"
would not let us differenciate the products of 1/3 and 2/3 X-<.

> ---

We need two Signed-off-by: lines at the end, one by Wyman and then
another by you in this order.

> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt

Changes to this file in this patch looks sane.

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1260,6 +1262,14 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>  			*s = xstrdup(remote);
>  		else
>  			*s = "";
> +	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
> +		int explicit, for_push = starts_with(atom->name, "push");
> +		const char *merge = remote_ref_for_branch(branch, for_push,
> +							  &explicit);

Having multiple variables defined like this _and_ initialized with
anything other than a trivial constant, is somewhat hard to follow.
Can we split the above more like:

		int explicit;
		int for_push = starts_with(...);
		const char *merge = remote_ref_for_branch(...);

Actually, if you did it this way, you do not even need "for_push":

		int explicit;
		const char *merge;

		merge = remote_ref_for_branch(branch,
					      starts_with(atom->name, "push"),
					      &explicit);

which may be a lot easier to follow.

By the way, the spirit of parsing used atoms first before the "learn
what we care about this single ref" function like the above are
called repeatedly for each ref is because we want to hoist the
overhead of doing the constant computation like "does the atom's
name begins with 'push'?" out of the latter.  

Can we add a field in atom->u.remote_ref to precompute this bit so
that we do not even have to say "Are we doing this for push?  Let's
see if the atom's name begins with 'push'" each time this function
is called for a ref?  That makes this function a lot more in line
with the spirit of the design of the subsystem, I would think.

This comment probably applies equally to both 1/3 and this one.

> +		if (explicit)
> +			*s = xstrdup(merge);
> +		else
> +			*s = "";

Here is the same "who are we trying to help---users who want to know
where their push goes, or users who are debugging where the push
destination is defined?" question.  I do not have a strong opinion
either way, but I think giving the end result with fallback (i.e.
not nullifying when the result is not explicit) may be easier to use
by "push" users.

> diff --git a/remote.c b/remote.c
> index b220f0dfc61..2bdcfc280cd 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -675,6 +675,36 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
>  	return remote_for_branch(branch, explicit);
>  }
>  
> +const char *remote_ref_for_branch(struct branch *branch, int for_push,
> +				  int *explicit)
> +{
> +	if (branch) {
> +		if (!for_push) {
> +			if (branch->merge_nr) {
> +				if (explicit)
> +					*explicit = 1;
> +				return branch->merge_name[0];
> +			}
> +		} else {
> +			const char *dst, *remote_name =
> +				pushremote_for_branch(branch, NULL);
> +			struct remote *remote = remote_get(remote_name);

Again,

			const char *dst, *remote_name;
                        struct remote *remote;

			remote_name = pushremote_for_branch(branch, NULL);
                        remote = remote_get(remote_name);

may be easier to follow, if only that it allows eyes to scan without
having to be distracted by jagged lines.

> +			if (remote && remote->push_refspec_nr &&
> +			    (dst = apply_refspecs(remote->push,
> +						  remote->push_refspec_nr,
> +						  branch->refname))) {
> +				if (explicit)
> +					*explicit = 1;
> +				return dst;
> +			}
> +		}
> +	}
> +	if (explicit)
> +		*explicit = 0;
> +	return "";
> +}

What the function does looks reasonable; the assignment in if()
condition looks a bit unfortunate, though.