Re: [PATCH 3/2] Enhance --early-output format

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Nov 5, 2007, 20:47 UTC
Message-ID: <alpine.LFD.0.999.0711051233270.15101@woody.linux-foundation.org>
In-Reply-To: <7vsl3kphjp.fsf@gitster.siamese.dyndns.org>

On Mon, 5 Nov 2007, Junio C Hamano wrote:

Show 17 quoted lines

>
> > +		if (rev->prune_fn && rev->dense && !(flags & TREECHANGE)) {
> > +			if (commit->parents && !commit->parents->next)
> > +				continue;
> > +		}
> 
> When looking at:
> 
> 	if (A && B && C) {
>         	if (D && E)
>                 	continue;
> 	}
> 
> an uninitiated might say "Huh?  Why use nested 'if'?", but to
> somebody who knows how revision traversal works, the above split
> is a more logical way to test this condition.  Maybe one liner
> comment is in order?

I'd almost prefer not to.

If people feel the code is subtle enough that a comment is in order to explain *what* something does, I would rather introduce helper functions than add comments. I'm not a big believer in comments in the middle of code to explain the code, but I *am* a big believer in trying to make the code easy to read without them.

(I don't dislike comments per se, but I'd *much* rather have the comments explain what is going on at a higher level. Comments that talk about the details of the code itself is likely bogus).

So we could add a commit to say what is going on ("ignore commits that have only one parent and didn't change the tree"), but I'd not want to explain why that particular layout of code.

That said, I've often found the "TREECHANGE" bit annoying. The fact that we always have to test it together with testing "rev->prune_fn" and often also the "rev->dense" flag is just annoying. I'd almost like to just always set the bit if "rev->prune_fn" isn't set. Alternatively, the code could be rewritten to just have a few inline helper functions, and it could perhaps be written as

	if (!(flags & TREECHANGE) && revs->dense && single_parent(commit))
		continue;

I dunno. I have no really *strong* opinions, although I suspect that anybody who looks at that particular piece of code had better understand these issues well enough anyway that it doesn't much matter..

Show 6 quoted lines

> As you noted, this is more like "Partial output" now.
> How about painting the bikeshed pink by saying:
> 
> 	Partial output: 20
>         Partial output: 70
>         Final output: 70000

I'm fine with it. The reason I did it the way I did was that this way I didn't need to change "gitk" - I could just use Pauls original patch as-is, since that code didn't care *what* came after the "Final output", and didn't care how many times it showed up.

			Linus