Re: [PATCH 2/3] for-each-ref: let upstream/push optionally report merge name.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 4, 2017, 09:12 UTC
- Message-ID
- <xmqq4lrfb7jk.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <f615fcc05fa358b4c7e3531cbdbd91661be321aa.1506952571.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 32 quoted lines
> 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.
Show 10 quoted lines
> --- 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.
Show 22 quoted lines
> 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.
Show 14 quoted 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.