git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: git@vger.kernel.org, J Wyman <jwyman@microsoft.com>
Subject: Re: [PATCH 2/3] for-each-ref: let upstream/push optionally report merge name.
Date: Wed, 04 Oct 2017 18:12:47 +0900
Message-ID: <xmqq4lrfb7jk.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <f615fcc05fa358b4c7e3531cbdbd91661be321aa.1506952571.git.johannes.schindelin@gmx.de>

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.

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-02 13:56 [PATCH 0/3] for-each-ref: add :remote-ref and :remote-name specifiers Johannes Schindelin
2017-10-02 13:56 ` [PATCH 1/3] for-each-ref: let upstream/push optionally report the remote name Johannes Schindelin
2017-10-04  7:14   ` Junio C Hamano
2017-10-04  9:08     ` Junio C Hamano
2017-10-05 12:20     ` Johannes Schindelin
2017-10-02 13:57 ` [PATCH 2/3] for-each-ref: let upstream/push optionally report merge name Johannes Schindelin
2017-10-04  9:12   ` Junio C Hamano [this message]
2017-10-04 11:41     ` Junio C Hamano
2017-10-05  9:14       ` Junio C Hamano
2017-10-02 13:57 ` [PATCH 3/3] for-each-ref: test :remote-name and :remote-ref Johannes Schindelin
2017-10-05  1:54 ` [PATCH 0/3] for-each-ref: add :remote-ref and :remote-name specifiers Junio C Hamano
2017-10-05 12:19 ` [PATCH v2 0/3] for-each-ref: add :remoteref and :remotename specifiers Johannes Schindelin
2017-10-05 12:19   ` [PATCH v2 1/3] for-each-ref: let upstream/push optionally report the remote name Johannes Schindelin
2017-10-10  9:35     ` Junio C Hamano
2017-10-11  2:07     ` Junio C Hamano
2017-10-05 12:19   ` [PATCH v2 2/3] for-each-ref: let upstream/push optionally remote ref name Johannes Schindelin
2017-10-06  5:10     ` Junio C Hamano
2017-10-11  5:58       ` Junio C Hamano
2017-10-12 19:13         ` Johannes Schindelin
2017-10-13  0:30           ` Junio C Hamano
2017-10-15 16:02             ` Johannes Schindelin
2017-10-13 16:39     ` Kevin Daudt
2017-10-14  2:08       ` Junio C Hamano
2017-10-15 16:05         ` Johannes Schindelin
2017-10-16  1:50           ` Junio C Hamano
2017-10-16 11:39             ` Johannes Schindelin
2017-10-05 12:19   ` [PATCH v2 3/3] for-each-ref: test :remotename and :remoteref Johannes Schindelin
2017-11-07 16:30   ` [PATCH v3 0/3] for-each-ref: add :remoteref and :remotename specifiers Johannes Schindelin
2017-11-07 16:30     ` [PATCH v3 1/3] for-each-ref: let upstream/push optionally report the remote name Johannes Schindelin
2017-11-07 16:31     ` [PATCH v3 2/3] for-each-ref: let upstream/push report the remote ref name Johannes Schindelin
2017-11-07 16:31     ` [PATCH v3 3/3] for-each-ref: test :remotename and :remoteref Johannes Schindelin
2017-11-08  1:36     ` [PATCH v3 0/3] for-each-ref: add :remoteref and :remotename specifiers Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq4lrfb7jk.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=johannes.schindelin@gmx.de \
    --cc=jwyman@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox