On Mon, Dec 13, 2021 at 7:09 AM William Sprent <williams@xxxxxxxxxxx> wrote:
>
> > 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.
> 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.
> 3. Keep the reverse assignment at the bottom, silently ignoring any
> --reverse option.
"silently ignored" or "dismissed with prejudice"? :-)
> 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...
> 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.
> >> +
> >> + 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.
