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.
next prev parent reply index Thread overview: 31+ 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