[PATCH 3 of 3 V2] revsets: add new toposort predicate

Pierre-Yves David pierre-yves.david at ens-lyon.org
Wed May 18 20:29:53 EDT 2016



On 05/18/2016 06:15 PM, Martijn Pieters wrote:
> On 18 May 2016 at 15:44, Pierre-Yves David
> <pierre-yves.david at ens-lyon.org> wrote:
>> Why is this a new predicate instead of a key for the 'sort' revset? This
>> would seems more appropriate.
> sort() can sort on more than one key; e.g. sort(set, "-date author")
> to sort in reverse date order (newest first), then within each date,
> by author in alphabetical order. It makes no sense to apply a toposort
> to any of the other sort orders; note that the algorithm requires the
> revisions to be sorted in revision order!
>
> Rather than cram toposort into the sort() but then have to explain
> that when you use 'topo' as a sort key the other keys must be ignored,
> it was much cleaner to create a separate predicate.

I'm not really convinced by that argument. Actually I think it does make
sense. The topological sort will eventually grow some way to break tie.
Being able to says things like "topo -date" would allow to express them
(here, sort more recent first in case of tie). So using the "sort()"
predicate is actually helping to solve one of our UI challenge with
topological sorting.
However, we should probably disallow "topo" anywhere but in first
position (at least at first).

> toposort() also needs to accept the firstbranch argument; it would
> have required adding a third to sort() that only applies to just one
> type of sort.

I find this argument a bit more convincing. Having a parameter for a
single case would be inelegant (not impossible but not super excited
about it).

Yet, maybe we can express this parameters in a way that is useful for
other kind of sorting. If we turn this into some kind of manual priority
it would make senses for others:

- topo → branch(es) to display first,
- author → manual list of priority/order for author name. Useful to put
oneself first,
- branch → idem, having the ability to manually sort branch (eg: stable
>> default).

This would be especially useful in combination with the ability to
provide multiple sort key (eg "topo author") as you could very precisely
specify how to break tie.

I'm not saying we should implement all of this before taking toposort,
but if we can have a whole/global story for an extra argument we could
consider this route.

That said, there is probably other parameters we might want to implement
in the future. For exaple some limit to the size of a topological
grouped series of revs (to avoid sorting too far down in case of -very-
long mixed branches). Having such parameter may not make sense for most
of the key (or would deserve different default). This is more a generic
statement about how we could have more option than a specific example to
study.

>
>> Could we move the code around in an independant patch this would make
>> actual new code introduction and change easier to track.
> I adapted your code slightly to fit a predicate. I'm not sure that the
> move from graphmod.py to revset.py makes any sense without the
> changes.

I think that would significantly improve the reviewability of this
series of the move and the change were splitted. I am fine with a
changeset moving the function elsewhere even if the code is still only
called in the graphmod module. Can you give it a try?

Cheers,

-- 
Pierre-Yves David




More information about the Mercurial-devel mailing list