Re: [PATCH 1/5] Introduces for_each_revision() helper

From: Shawn O. Pearce <spearce@spearce.org>
Date: Apr 29, 2007, 07:06 UTC
Message-ID: <20070429070616.GV5942@spearce.org>
In-Reply-To: <7vy7kbeke1.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:

Show 7 quoted lines

> The reason I do not like this particular one is because both
> operations you are hiding are not simple operations like
> "initialize a variable to list head" or "follow a single pointer
> in the structure", but rather heavyweight operations with rather
> complex semantics.  I would want to make sure that people
> realize they are calling something heavyweight when they use the
> revision traversal.

But in_merge_base is heavyweight if the two commits are in the same object database, but aren't connected at all. You'll need to traverse both histories before aborting and saying there is no merge base. That ain't cheap on large trees. But its also a single line of code.

Anyway, my original problem with this macro was the way it was defined. I think Luiz was able to fix most of my issues with it in his latest version, but I still have a personal distaste for hiding things like a for(;;) construct in a macro, or allowing a macro parameter to be used more than once within the definition of the macro (unexpected side-effects of evaluating an more than once).

-- 
Shawn.