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

 On Thu, Dec 9, 2021 at 12:13 AM William Sprent via GitGitGadget
[off-list ref] wrote:

From: William Sprent <redacted>

When invoking git-fast-export with the --first-parent flag on a branch
with merges, fast-export would early-out on processing the first merge
on the branch. If combined with --reverse, fast-export would instead
output all single parent commits on the branch.

This commit makes fast-export output the same commits as rev-list
--first-parent, and makes --reverse not have an effect on which commits
are output.

The fix involves removing logic within fast-export which was responsible
for ensuring that parents are processed before their children, which was
what was exiting early due to missing second parents. This is replaced
by setting 'reverse = 1' before revision walking, which, in conjuction
with topo_order, allows for delegating the ordering of commits to
revision.c. The reverse flag is set after parsing rev-list arguments to
avoid having it disabled.

This explains how you discovered the issue, but seems to focus on
these particular flags, leaving the reader wondering whether there are
other flags that are also mishandled that need additional care, and
whether this fix might break things for other sets of flags.  It took
me quite a bit of reasoning to satisfy myself on those points; it's
actually somewhat complex.  May I suggest an alternative commit
message based on that?  Here's what I think are the relevant points
(and yeah, it's lengthy):


The revision traversal machinery typically processes and returns all
children before any parent.  fast-export needs to operate in the
reverse fashion, handling parents before any of their children in
order to build up the history starting from the root commit(s).  This
would be a clear case where we could just use the revision traversal
machinery's "reverse" option to achieve this desired affect.

However, this wasn't what the code did.  It added its own array for
queuing.  The obvious hand-rolled solution would be to just push all
the commits into the array and then traverse afterwards, but it didn't
quite do that either.  It instead attempted to process anything it
could as soon as it could, and once it could, check whether it could
process anything that had been queued.  As far as I can tell, this was
an effort to save a little memory in the case of multiple root commits
since it could process some commits before queueing all of them.  This
involved some helper functions named has_unshown_parent() and
handle_tail().  For typical invocations of fast-export, this
alternative essentially amounted to a hand-rolled method of reversing
the commits -- it was a bunch of work to duplicate the revision
traversal machinery's "reverse" option.

This hand-rolled reversing mechanism is actually somewhat difficult to
reason about.  It takes some time to figure out how it ensures in
normal cases that it will actually process all traversed commits
(rather than just dropping some and not printing anything for them).

And it turns out there are some cases where the code does drop commits
without handling them, and not even printing an error or warning for
the user.  Due to the has_unshown_parent() checks, some commits could
be left in the array at the end of the "while...get_revision()" loop
which would be unprocessed.  This could be triggered for example with
    git fast-export main -- --first-parent
or non-sensical traversal rules such as
    git fast-export main -- --grep=Merge --invert-grep

While most traversals that don't include all parents should likely
trigger errors in fast-export (or at least require being used in
combination with --reference-excluded-parents), the --first-parent
traversal is at least reasonable and it'd be nice if it didn't just
drop commits.  It'd also be nice to have a simpler "reverse traversal"
mechanism.  Use the "reverse" option of the revision traversal
machinery to achieve both.

Even for the non-sensical traversal flags like the --grep one above,
this would be an improvement.  For example, in that case, the code
previously would have silently truncated history to only those commits
that do not have an ancestor containing "Merge" in their commit
message.  After this code change, that case would would include all
commits without "Merge" in their commit message -- but any commit that
previously had a "Merge"-mentioning parent would lose that parent
(likely resulting in many new root commits).  While the new behavior
is still odd, it is at least understandable given that
--reference-excluded-parents is not the default.

Signed-off-by: William Sprent <redacted>
---
    fast-export: fix surprising behavior with --first-parent

    Hi,

    I've noticed that git fast-export exhibits some odd behavior when passed
    the --first-parent flag. In the repository I was working on, it would
    only output the initial commit before exiting. Moreover, adding the
    --reverse flag causes it to behave differently and instead output all
    commits in the first parent line that only have one parent. My
    expectation is more or less that git fast-export should output the same
    set of commits as git rev-list would output given the same arguments,
    which matches how it acts when given revision ranges.

    It seems like this behavior comes from the fact that has_unshown_parents
    will never be false for any merge commits encountered when fast-export
    is called with --first-parent. This causes the while loop to follow the
    pattern of pushing all commits into the "commits" queue until the
    initial commit is encountered, at which point it calls handle_tail which
    falls through on the first merge commit, causing most of the commits to
    be unhandled.

    My impression is that this logic only serves to ensure that parents are
    processed before their children, so in my patch I've opted to fix the
    issue by delegating that responsibility to revision.c by adding the
    reverse flag before performing the revision walk. From what I can see,
    this should be equivalent to what the previous logic is trying to
    achieve, but I can also see that there could be risk in these changes.

    Changes since v1:

     * Moved revs.reverse assignment down to similar assignments below.
     * Removed braces around single statement while loop.
     * The test now only changes directory inside a sub-shell.
     * Applied stylistic feedback on test such as: making redirections be on
       the form >FILE etc.

    There were questions raised about whether it makes sense at all for
    fast-export to simply accept all git rev-list arguments whether they
    have an effect or not - in particular for a flag like --reverse. I think
    I agree that it is questionable behavior, or at least questionably
    documented, but I also think it is out of scope for this change.

    I did consider teaching fast-export to complain if given --reverse, but
    it seemed inconsistent to me as it will gladly accept every other
    rev-list argument (for example, "git fast-export HEAD --show-signature
    --graph" works just fine).

    cc: Ævar Arnfjörð Bjarmason avarab@gmail.com

...

Deleted code is debugged code!  :-)

I really wanted to see this next to the
    rev.topo_order = 1
statement elsewhere, but as you alluded to in the commit message, this
part of revision.c makes that unsafe:

"""
} else if (!strcmp(arg, "--reverse")) {
    revs->reverse ^= 1;
"""

However, given that it's unsafe to set revs.reverse=1 earlier, now
that I think about it, isn't it also unsafe to set revs.topo_order
where it is?  Someone could override it by passing --date-order to
fast-export.  (It's okay if you want to leave fixing that to someone
else, just thought I'd mention it while reviewing.)

        revs.diffopt.format_callback = show_filemodify;
        revs.diffopt.format_callback_data = &paths_of_changed_objects;
        revs.diffopt.flags.recursive = 1;
-       while ((commit = get_revision(&revs))) {
-               if (has_unshown_parent(commit)) {
-                       add_object_array(&commit->object, NULL, &commits);
-               }
-               else {
-                       handle_commit(commit, &revs, &paths_of_changed_objects);
-                       handle_tail(&commits, &revs, &paths_of_changed_objects);
-               }
-       }
+       while ((commit = get_revision(&revs)))
+               handle_commit(commit, &revs, &paths_of_changed_objects);

Looks good.  Nice work on finding this.

micro nit: I'd really prefer the --no-ff before the "topic1" and "topic2".

+
+               test_commit D &&
+
+               git rev-list --format="%ad%B" --first-parent --topo-order --no-commit-header main >expected &&

I'd much prefer this were changed to
    git log --format="%ad %s" --first-parent >expected &&
because:
  * --topo-order is irrelevant when you have a linear history (which
--first-parent gives you)
  * --no-commit-header can be tossed by using log instead of rev-list
  * %B provides the entire (multi-line) commit message body but you
clearly care about how many commits you saw below and were assuming
just one line per commit, so %s is better.
  * The format looks weird when inspecting as a human; it's much nicer
with a space between the %ad and %s.

+
+               git fast-export main -- --first-parent >first-parent-export &&
+               git fast-export main -- --first-parent --reverse >first-parent-reverse-export &&
+
+               git init import &&
+               git -C import fast-import <first-parent-export &&
+
+               git -C import rev-list --format="%ad%B" --topo-order --all --no-commit-header >actual &&

Same simplifications as above here:
   git -C import log --format="%ad %s" --topo-order --all >actual &&

However, is there a reason you're using "--all" instead of "main"?
Although main is the only branch, which makes either output the same
thing, it'd be easier for folks reading to catch the equivalence if it
was clear you were now listing information about the same branch.

+               git -C import rev-list --all >tmp &&
+
+               test_line_count = 4 tmp &&

I don't understand why you need tmp.  Why not just use actual on the
test_line_count line?

+               test_cmp expected actual &&
+               test_cmp first-parent-export first-parent-reverse-export

This last line would be much nicer to include right after
first-parent-reverse-export is created.  Or else move the creation of
first-parent-reverse-export down to just above this line.

+       )
+'
+
 test_done