Re: [PATCH] show-branch: show all local heads when only giving one rev along --topics

2015-03-30 Thread Junio C Hamano 
Mike Hommey m...@glandium.org writes:

 (Maybe --topics should always require one rev on the command
 line?)

That sounds line a good thing to do.

 - else if (all_heads + all_remotes)
 - snarf_refs(all_heads, all_remotes);
   else {
   while (0  ac) {
   append_one_rev(*av);
   ac--; av++;
   }
 + if (all_heads + all_remotes)
 + snarf_refs(all_heads, all_remotes);

Hmm.  Is this safe and will not cause problems by possibly
duplicated refnames that came from the command line and the ones
that came from for-each-ref iteration?  I am not saying the change
is problematic; it is just I haven't looked at this code for a long
time that the existing machinery is already designed to tolerate
duplicated input.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] show-branch: show all local heads when only giving one rev along --topics

2015-03-30 Thread Mike Hommey 
On Mon, Mar 30, 2015 at 03:24:26PM -0700, Junio C Hamano wrote:
 Mike Hommey m...@glandium.org writes:
 
  (Maybe --topics should always require one rev on the command
  line?)
 
 That sounds line a good thing to do.
 
  -   else if (all_heads + all_remotes)
  -   snarf_refs(all_heads, all_remotes);
  else {
  while (0  ac) {
  append_one_rev(*av);
  ac--; av++;
  }
  +   if (all_heads + all_remotes)
  +   snarf_refs(all_heads, all_remotes);
 
 Hmm.  Is this safe and will not cause problems by possibly
 duplicated refnames that came from the command line and the ones
 that came from for-each-ref iteration?  I am not saying the change
 is problematic; it is just I haven't looked at this code for a long
 time that the existing machinery is already designed to tolerate
 duplicated input.

It is:
https://github.com/git/git/blob/master/builtin/show-branch.c#L382

In case you wonder about allow_dups, the only case in which it's 1 is:
https://github.com/git/git/blob/master/builtin/show-branch.c#L784
which is the reflog case, which is the case before that `else` in the
patch.

That is, both append_one_rev and snarf_refs end up calling append_ref
with allow_dups=0.

Cheers,

Mike
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] show-branch: show all local heads when only giving one rev along --topics

2015-03-30 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

 ...?  I am not saying the change
 is problematic; it is just I haven't looked at this code for a long
 time that the existing machinery is already designed to tolerate
 duplicated input.

for a long time to say that the existing code is OK or not is what
I meant to say.  Sorry for the noise.

 Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] show-branch: show all local heads when only giving one rev along --topics

2015-03-16 Thread Mike Hommey 
On Mon, Mar 16, 2015 at 05:38:06PM +0900, Mike Hommey wrote:
 git show-branch --topics rev revs... displays ancestry graph, only
 considering commits that are in all given revs, except the first one.
 
 git show-branch displays ancestry graph for all local branches.
 
 Unfortunately, git show-branch --topics rev only prints out the rev
 info for the given rev, and nothing else, e.g.:
 
   $ git show-branch --topics origin/master
   [origin/master] Sync with 2.3.3
 
 While there is an option to add all remote-tracking branches (-r), and
 another to add all local+remote branches (-a), there is no option to add
 only local branches. Adding such an option could be considered, but a
 user would likely already expect that the above command line considers
 the lack of rev other than for --topics as meaning all local branches,
 like when there is no argument at all.
 
 Moreover, when using -r and -a along with --topics, the first local or
 remote-tracking branch, depending on alphabetic order is used instead of
 the one given after --topics (any rev given on the command line is
 actually simply ignored when either -r or -a is given). And if no rev is
 given at all, the fact that the first alphetical branch is the base of
 topics is probably not expected by users (Maybe --topics should always
 require one rev on the command line?)
 
 This change makes
   show-branch --topics $rev
 act as
   show-branch --topics $rev $(git for-each-ref refs/heads
--format='%(refname:short)')
 
   show-branch -r --topics $rev ...
 act as
   show-branch --topics $rev ... $(git for-each-ref refs/remotes
--format='%(refname:short)')
 instead of
   show-branch --topics $(git for-each-ref refs/remotes
   --format='%(refname:short)')
 
 and
   show-branch -a --topics $rev ...
 act as
   show-branch --topics $rev ... $(git for-each-ref refs/heads refs/remotes
--format='%(refname:short)')
 instead of
   show-branch --topics $(git for-each-ref refs/heads refs/remotes
   --format='%(refname:short)')

Sorry, this was missing:
Signed-off-by: Mike Hommey m...@glandium.org

Mike
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html