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

On 14/12/2021 01.31, Elijah Newren wrote:

On Mon, Dec 13, 2021 at 7:09 AM William Sprent [off-list ref] wrote:

quoted

quoted

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

I couldn't tell you for sure if the topo_order placement is safe. I at
least don't see any place where topo_order itself can be toggled off in
revision.c.  I'm sure there exists at least one rev-list argument which
will cause unexpected behaviour, though.

I agree that it would be nice to have the traversal order options be
assigned in the same place. I guess we have three options:


     1. Put the reverse assignment to the top (together with topo_order),
allowing the user to disable it with --reverse, which will cause odd
behaviour.

I'd call it broken rather than merely odd; more on this below.

quoted

     2. Put the reverse assignment to the top and throw an error if the
user passes the --reverse option.

Might be a reasonable longer term solution if someone wants to dive
into all the non-sensical options and mark them as such.  But I agree
that it's slightly odd only picking one specific one when we know
there's likely a pile of them here.

quoted

     3. Keep the reverse assignment at the bottom, silently ignoring any
--reverse option.

"silently ignored" or "dismissed with prejudice"?  :-)

Heh. :) It would definitely an "interesting" option to be able to reverse the commit graph, if it worked..

quoted

I don't think any of the three options are particularly good. The first
one for obvious reasons. The second seems inconsistent to me as we would
only error on --reverse but not any of the other "nonsensical" rev-list
args. However, silently ignoring certain arguments does also not make
for a good user experience.

I think that it might be a good idea to move up the 'reverse' assignment
and then add a paragraph to the man page for git-fast-export explaining
that some arguments, in particular the ones that change the ordering of
commits and the ones that change how commits are displayed (such as
--graph), may have no or unexpected effects.

I'd rather choose option #3, like builtin/add.c does with max_count.
In part this is because...

quoted

I've tried writing a snippet in git-fast-export.txt, which I could include
in the next version, if you think it seems like a reasonable approach:

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 1978dbdc6a..34875ef01d 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt

@@ -157,16 +157,21 @@ by keeping the marks the same across runs.
  [<git-rev-list-args>...]::
         A list of arguments, acceptable to 'git rev-parse' and
         'git rev-list', that specifies the specific objects and references
         to export.  For example, `master~10..master` causes the
         current master reference to be exported along with all objects
         added since its 10th ancestor commit and (unless the
         --reference-excluded-parents option is specified) all files
         common to master{tilde}9 and master{tilde}10.
++
+Arguments to `git rev-list` which change the _order_ in which commits are
+traversed, such as '--reverse', as well as arguments which control how commits
+are displayed, such as '--graph', may either have no effect or have an
+unexpected effect on which commits are exported.

After your patch, --reverse won't have an unexpected effect on _which_
commits are exported, it would instead have an unexpected effect on
_how_ commits are exported (turning _every_ commit into a root
commit).  I'd rather just go with your option #3.

Alright. I guess another solution would have been to move topo_order down, but that seems unsafe as well. According to your commit, 668f3aa776 (fast-export: Set revs.topo_order before calling setup_revisions, 2009-06-25).

I'll leave the revs.reverse assignment where it is for now.

quoted

quoted

quoted

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

I guess the intent is to be completely sure that only four commits were
exported, and no other refs made it into the new repository. I don't feel
too strongly about it, but I think it is a slightly stronger test than
leaving out the '--all'.

Fair enough, '--all' works for me with that explanation.

Ok. In summary I'll use the commit message you wrote and apply the rest of your feedback for the test for the next version.