Re: [RFC] Making pathspec limited log play nicer with --first-parent

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 19, 2012, 20:43 UTC
Message-ID: <7vwr8niftt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CA+55aFxucaeX7it_Kj7WV3ZbwCukN+wvbuxqJzh3V5Rxz4ib1g@mail.gmail.com>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 19 quoted lines

> On Thu, Jan 19, 2012 at 11:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Comments?
>
> Looks conceptually right, but I do have to admit to hating that new variable.
>
> I don't see a better way to do it, though. Sure, you could do it with just
>
>    if (revs->first_parent_only && pp != &commit->parents)
>              break;
>
> and avoid the new variable that way, but that replaces the annoying
> variable with a pretty subtle thing.
>
> Or we could re-write that while() loop and move the 'parent' variable
> into it. Like the appended untested thing.
>
> But maybe your patch is better, and my dislike for that parent counter
> is just irrational.

I didn't like that parent counter that _only_ increments when we are running under first-parent-only mode at the conceptual level. At the implementation level, of course it is the right thing to do because outside first-parent-only mode nobody cares about the parent counter, so it is a valid but subtle optimization.

But I personally find your loop

	do {
        	...
	} while (!revs->first_parent_only);

is even more disgusting. It is misleading to have something that is not supposed to change inside the loop as the terminating condition as if we are saying "loop until somebody flips that bit" which is clearly not the case.

So obviously I am saying that I do not think either patch is pretty without offering a better alternative implementation, which is my usual badness. As this is not an ultra urgent fix, I'll wait and see if somebody else comes up with a more readable version.

Thanks for eyeballing the logic side of it, anyway. That was what I was worried about the change the most.