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

[William Sprent <williams@unity3d.com>]

On 14/12/2021 01.31, Elijah Newren wrote:
> On Mon, Dec 13, 2021 at 7:09 AM William Sprent <williams@unity3d.com> 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"?  :-)
> 

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

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

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.

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