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

[Martijn Pieters]

On 19 May 2016 at 01:29, Pierre-Yves David
<pierre-yves.david at ens-lyon.org> wrote:
>> 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.

Interesting, that does make a compelling argument to make 'topo' a
special kind of `sort()` key.

But the next question then would be: how should per-sort-key arguments
then be expressed?

I wonder if predicates could support *keyword arguments* here:

    sort(set, "author -date', author=mjpieters at fb.com)

would sort by author first (putting me at the top), then by reverse
date for example. Each sort key accepts just one keyword argument, and
sort keys must be unique (sorting by the same key twice makes no sense
anyway).

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

Going along with the keyword arguments, you could extend this to use prefixes:

    topo_firstbranch=...
    topo_grouplimit=...

    author_first=...

    branch_first=...

etc.

That said, expanding the parser to support keyword arguments is a
little out of my reach and time budget. *If* I were to merge
toposort() into sort(), would using the 3rd argument work until such
time that alternative syntax is found (after which a 3rd argument is,
for a while, an alias for the new syntax spelling)?

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

I'll give it a punt, look out for a V2 soon.

-- 
Martijn Pieters