Re: [PATCH v2] revision.c: really honor --first-parent

["Lars Hjemli" <hjemli@gmail.com>]

On Tue, May 13, 2008 at 10:15 PM, Stephen R. van den Berg <srb@cuci.nl> wrote:
> Lars Hjemli wrote:
>  >In add_parents_to_list, if any parent of a revision had already been
>  >SEEN, the current code would continue with the next parent, skipping
>  >the test for --first-parent. This patch inverts the test for SEEN so
>  >that the test for --first-parent is always performed.
>
>  Let's put it this way:
>  - If there would have been only one path to any particular point in the
>   tree, then the --first-parent flag makes no differences, because the
>   tree wouldn't contain any merges to begin with.

True

>  - If a tree contains *any* merges (i.e. a commit with multiple parents),
>   then there are always multiple paths to some common ancestor, and
>   therefore depending on which path you travel up first, you sometimes get
>   clashes with the SEEN flag (unpredictable by definition).

True

>  - It would seem logical and sufficient to avoid this unpredictability by
>   utilising the --first-parent flag to present and walk a tree of commits
>   AS IF there were no merges.

True

>  - My original patch did just that, it simplified the code to make sure
>   that all other parents beside the first parent are ignored when
>   walking the tree.

Except for the case where the first parent had been already SEEN; then
it would continue to test the next parents until one was found which
was not already SEEN and _that_ parent would be treated as if it was
first. And as Nanako showed, a simple `git rev-list HEAD^..HEAD` marks
both HEAD and HEAD^ as seen. When combined with --first-parent, the
result (with your patch) is that HEAD^2 is treated as the first
parent. With my patch on top of yours, the walk stops as HEAD^, which
is what we probably both want.

>  - Your code now doesn't simplify the (IMO) convoluted walk, and still
>   marks things as seen, even though in the first-parent case, these
>   commits are not really seen at all.  It implies that your code
>   generates differing output, depending on the merges present.

I don't think so. My code should neither follow nor mark as SEEN any
parent but the first (but I could obviously be wrong).


>  - The question now is, do we want the output of --first-parent to be
>   immutable with respect to merges being present (but hidden from sight
>   during a --first-parent run), or do we want the output of
>   --first-parent to actually change depending on variations in parents
>   other than the first parent?
>
>  I'd say it's better to keep the code simpler, and to make sure the
>  output does *not* depend on any parents other than the first (as
>  implemented in my original patch).

I agree with your reasoning, and your patch with mine on top seems to
achieve that goal.

--
larsh