[PATCH 1 of 2] revset: add topo.revsonly argument for topo sort

[Pierre-Yves David]

On 09/24/2016 12:28 AM, Xidorn Quan wrote:
> On Sat, Sep 24, 2016, at 08:16 AM, Pierre-Yves David wrote:
>> On 09/24/2016 12:10 AM, Xidorn Quan wrote:
>>> On Sat, Sep 24, 2016, at 08:07 AM, Pierre-Yves David wrote:
>>>> On 09/24/2016 12:01 AM, Xidorn Quan wrote:
>>>>> On Sat, Sep 24, 2016, at 05:47 AM, Pierre-Yves David wrote:
>>>>>> On 09/23/2016 03:26 PM, Xidorn Quan wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Xidorn Quan <me at upsuper.org>
>>>>>>> # Date 1474636628 -36000
>>>>>>> #      Fri Sep 23 23:17:08 2016 +1000
>>>>>>> # Node ID 9e8aeaddddf3bf6e61b351a738f5cadbbf4815f3
>>>>>>> # Parent  285a8c3e53f2183438f0cdbc238e4ab851d0d110
>>>>>>> revset: add topo.revsonly argument for topo sort
>>>>>>>
>>>>>>> This argument is used when we want an order which is not affected by
>>>>>>> revisions other than given ones. It helps the next patch (rebase in
>>>>>>> topo order) to keep sensible order when multiple separate revisions
>>>>>>> are specified.
>>>>>>
>>>>>> I do not understand why this is necessary. How is the current code
>>>>>> misbehaving in your case?
>>>>>
>>>>> I did this for fixing "Test multiple root handling" in
>>>>> test-rebase-obsolete.t. But rejudging that testcase, it seems to me I
>>>>> probably should not do this, but instead, just update the result of that
>>>>> test with my second patch.
>>>>
>>>> What is the failure about?
>>>
>>> Please see the latest patch I sent to the list.
>>
>> Can you explain that change to me ? (and then add this explanation in
>> the commit description)
>
> That test is trying to rebase three different commits 7+11+9 on to 4.
> None of them are directly connected, but 11 is a descendant of 7. With
> the old behavior, the three commits would be rebased in 7,9,11 order,
> and after my change, 7 and 11 would be grouped together, so the order
> would be 9,7,11.
>
> I thought that all three commits would become immediate children of 4
> given they are not connected, and we should use revision order rather
> than topo order for separate subgraph.
>
> But it seems I was wrong. After the rebase, rebased 7 and 11 are still
> in the same branch, which means they really should be grouped together.

Okay, thanks for the explanation, It makes a lot of sense and I'm happy 
that asking the question helped to clarify the situation

>> I appreciate your enthousiasm to get this improved (and this is
>> definitely valuable to do so), but please avoid sending new version of
>> the patch when the previous one is still discussed. This get confusing
>> on the reviewers side.
>
> Oops, sorry about that.

Nothing too terrible either. Can you send a V4 with a small explanation 
about this change to an existing tests in the changeset description ?

Please also update the first line of the description to contains "(BC)" 
(to flag the small behavior change) and "(issue5370)" (to automatically 
close your issue on the tracker.

Cheers,

-- 
Pierre-Yves David