From: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Date: Wed, 03 Feb 2016 12:35:58 GMT
Subject: Re: [PATCH] log -g: ignore revision parameters that have no reflog
Message-ID: <1454502958.2713.13.camel@kaarsemaker.net>
In-Reply-To: <xmqqegcuprrw.fsf@gitster.mtv.corp.google.com>

On di, 2016-02-02 at 16:21 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > +	if (revs->reflog_info) {
> > +		/*
> > +		 * The reflog iterator gets confused when fed
> > things that don't
> > +		 * have reflogs. Help it along a bit
> > +		 */
> > +		if (strchr(arg, '@') != arg &&
> 
> Is this merely an expensive way to write *arg != '@', or is there
> something else I am missing?

Doh. No, that's just my stupidity. I did the strchrnul bits below
first, then found out that it broke `git log -g @{0}` and came up with
the above.

> > +		    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1,
> > &dotdot))
> > +			die("only refs can have reflogs");
> 
> Is "foo@23" a forbidden branch name?

It is not, the code should look for @{, not @.

> Is this looking for a dotdot?  If you are introducing a new scope,
> you can afford to invent a variable with a name that reflects its
> purpose.

True. I just adhered to surrounding style (the dotdot variable is
abused below as well). Lame excuse, I know :)

> Style: a binary operation like '-' (subtract) have SP on both sides
> of it.
> 
> > +		if(!reflog_exists(dotdot))
> 
> Style: one SP between a syntactic keyword like 'if' and opening
> parenthesis is required.

Ack.

> I have a suspicion that in your final "fixed" code, it may be a
> better design not to let the command line argument for "-g"
> processing pass through this function at all.
>
> For example, what should "git log -g master next" do?  Merge two
> reflog entries in chronological order and show each of them as if
> they are thrown at "git show" one by one?  Does that mesh well with
> other options like "--date-order/--topo-order"?

I agree that option parsing is not the right place in the end. When -g
is given, only one ref argument should be accepted, and --date-order
etc. should cause it to barf as they don't make sense.

> For another example, what should "git log -g master..next" do?
> 
> Or "git log -g master^^^"?
>
> These are merely a few example inputs I can think of off in 5
> seconds and I think none of the above makes much sense, but parsing
> these is the primary purpose of this function.

With this patch they die with an error as they make no sense.

> So, I dunno.  I gave a few "coding" comments, but I am not sure if
> you are touching the right codepath in the first place.

I was trying to go for a minimal change to fix a bug without
introducing regressions. It feels weird to do it in the option parsing
code, but I didn't want to make this behaviour fix wait for a rewrite
of the log -g functionality, as I have no idea when I'll be able to
finish that. It already took me a few hours to come up with this, as I
had not touched the related code at all before :)

-- 
Dennis Kaarsemaker
www.kaarsemaker.net