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

From: Mike Hommey <mh@glandium.org>
Date: Mar 30, 2015, 22:37 UTC
Message-ID: <20150330223752.GA19987@glandium.org>
In-Reply-To: <xmqqd23qw2cl.fsf@gitster.dls.corp.google.com>

On Mon, Mar 30, 2015 at 03:24:26PM -0700, Junio C Hamano wrote:

Show 23 quoted lines

> Mike Hommey <mh@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);
> 
> Hmmmmmm.  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