On 23/11/2021 20.14, Elijah Newren wrote:
On Tue, Nov 23, 2021 at 5:25 AM William Sprent via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
From: William Sprent <williams@xxxxxxxxxxx>
...
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.
