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

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

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