From: William Sprent Date: Wed, 24 Nov 2021 13:05:28 GMT Subject: Re: [PATCH] fast-export: fix surprising behavior with --first-parent Message-ID: <96bba3dd-589c-dc37-012a-6c827cafacb8@unity3d.com> In-Reply-To: On 23/11/2021 20.14, Elijah Newren wrote: > On Tue, Nov 23, 2021 at 5:25 AM William Sprent via GitGitGadget > wrote: >> >> From: William Sprent >> ... > > 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. > 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.