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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2016, 00:21 UTC
Message-ID: <xmqqegcuprrw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1454455961-10640-1-git-send-email-dennis@kaarsemaker.net>

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

Show 6 quoted lines

> +	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.