On Mon, 5 Nov 2007, Junio C Hamano wrote:
>
> > + 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..
> 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
