Re: [PATCH] fast-export: fix surprising behavior with --first-parent

From: William Sprent <williams@unity3d.com>
Date: Nov 24, 2021, 13:05 UTC
Message-ID: <96bba3dd-589c-dc37-012a-6c827cafacb8@unity3d.com>
In-Reply-To: <CABPp-BGm0VtU=z0r0oGc48J020Meh_xdibnagNeDmBGQ-RVu+g@mail.gmail.com>

On 23/11/2021 20.14, Elijah Newren wrote:

Show 23 quoted lines

> On Tue, Nov 23, 2021 at 5:25 AM William Sprent via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: William Sprent <williams@unity3d.com>
>> ...
> 
> Wow, interesting.  I did some related work that I never submitted at
> https://github.com/newren/git/commit/08f799b4667de1c755c78e1ea1657f946b588556
> -- in that commit, I also noticed that fast-export did not seem
> careful enough about making sure to process all commits, and came up
> with a much different fix.  However, in my case, I didn't know how to
> trigger the problems in fast-export without some additional changes I
> was trying to make, and since I never got those other changes working,
> I didn't think it was worth submitting my fix.  My solution is more
> complex, in part because it was designed to handle ordering
> requirements & recommendations other than just ancestry.  However, we
> don't currently have any additional ordering requirements (the current
> code only considers ancestry), so your solution is much simpler.  If I
> do at some point get my other changes working, I'd need to
> re-introduce the queue and handle_tail() and my changes to these, but
> that can always be done later if and when I get those other changes
> working.
>

Ah that is interesting. Good to know. I was happy that I could make fast-export rely on revision.c for ordering. At surface level, with the minimal amount of insight I have, that seems like an alright separation of concerns.

Show 12 quoted lines

> Interestingly, Dscho added both the --reverse ability to revision.c
> and the commits object_array and the handle_tail() stuff in
> fast-export.c.  Both were done in the same year, and --reverse came
> first.  See 9c5e66e97da8 (Teach revision machinery about --reverse,
> 2007-01-20) and f2dc849e9c5f (Add 'git fast-export', the sister of
> 'git fast-import', 2007-12-02).  It's not clear why revs.reverse = 1
> wasn't used previously, although it certainly didn't occur to me
> either and I think it would have been an alternative solution to my
> first ever git.git contribution -- 784f8affe4df (fast-export: ensure
> we traverse commits in topological order, 2009-02-10).  (Though
> rev.reverse = 1 wouldn't have provided the same speedups that
> rev.topo_order=1 provided.)

Ah. Thanks for the insight.

> I can't think of any cases where this would cause any problems, and it
> seems like using rev.reverse is a pretty clean solution.  Just in case
> I'm missing something, though, it would be nice to get Dscho to also
> comment on this.  I'm cc'ing him.

Thanks. Good idea.