Re: [PATCH] log -g: ignore revision parameters that have no reflog

[Junio C Hamano <gitster@pobox.com>]

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?

> +		    !dwim_ref(arg, strchrnul(arg, '@')-arg, sha1, &dotdot))
> +			die("only refs can have reflogs");

Is "foo@23" a forbidden branch name?

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.

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.

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"?

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.

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.