"git log --first-parent" shows parents that are not first

Quoting �������  <nanako3@bluebottle.com>:

Could you please revert d9c292e8bbd51c84cb9ecd86cb89b8a1b35a2a82? With that patch reverted from 'next', the problem disappears.

しらいしななこ  <nanako3@bluebottle.com> writes:

That's d9c292e (Simplify and fix --first-parent implementation, 2008-04-27) by Stephen.

I know that the alleged "fix" works around a corner-case, a fast-forward situation that was artificually recorded as a merge, but if the "cure" breaks a normal case like this, it is worse than the disease.

Stephen, do you have a fix?

In add_parents_to_list, if any parent of a revision had already been SEEN, the current code would continue with the next parent. But if the first parent has been SEEN and --first-parent has been specified we need to break, not continue.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 revision.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

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.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
This is a slightly different approach which I think is less ugly.

 revision.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Lars Hjemli wrote:

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.
- 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).
- 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.
- 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.
- 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.
- 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).

Your patch is smaller, and therefore (perhaps) less ugly; the resulting code and logic of my original patch is simpler (IMHO), and therefore cleaner (but it all depends on (the lack of) consensus over the points above).

On Tue, May 13, 2008 at 10:15 PM, Stephen R. van den Berg <srb@cuci.nl> wrote:

True

True

True

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.

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).

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

-- larsh

"Lars Hjemli" <hjemli@gmail.com> writes:

A major part of the "convoluted walk" is the (il-)logic that skipped earlier SEEN parents and treated the first unseen one as if it was the first parent, which is not exactly Stephen's fault. It was placed by yours truly in the very original code but it was done without much thought.

I think your patch is the correct fix for that convolution, regardless of the traversal order stability issue Stephen mentions.

Junio C Hamano wrote:

I agree that Lars' patch prevents parts of the tree to go "dark" (so did my patch). However, without either patch, that implies that the current --first-parent code has a high probability of obscuring parts of the tree depending on traversing order (in any tree which contains at least one merge).

So, I'd say, since the current code does not and cannot work reliably for anyone specifically using --first-parent (with every merge encountered, the probability of correctness is multiplied by 0.5 at most/least), you are going to do them a favour anyway by fixing the code, then why not simplify the convolution and make the code rock-steady (and implement my patch)?

Anyone using --first-parent in production now has an embarrassingly high probability of missing commits in his generated lists (I know that I noticed the problem within 5 minutes from actually trying to use the flag to get meaningful output). So fixing and simplifying the code now is rather unlikely to create any more surprises than the current code already presents to existing users (if any).

On Wed, May 14, 2008 at 12:34 PM, Stephen R. van den Berg <srb@cuci.nl> wrote:

The current 'next' branch in git.git contains your patch with my fixup on top and I believe this fixes _both_ the original issue with first-parent (thanks to your patch) and the issue Nanako discovered (thanks to my patch). Am I missing something?

Lars Hjemli wrote:

Probably not. I didn't check 'next' yet, since neither mine nor your patch had been Acked on the list (I guess it shows that I don't know the procedures here all too well yet).