( )⚙ D2668 rebase: introduce support for automatically rebasing orphan changes

This is an archive of the discontinued Mercurial Phabricator instance.

rebase: introduce support for automatically rebasing orphan changes
ClosedPublic

Authored by durin42 on Mar 4 2018, 3:37 PM.

Details

Summary

_destautorebase(SRC) is based on the _destrestack(SRC) revset from
fbamend. The supporting _possibledestination function is extracted
from evolve, with minor cleanups.

We've considered some alternatives here:

  • This change, but with --auto as the flag name. We're hedging our bets on this a little in this change so that if this ends up being the wrong direction we haven't burned the valauble --auto name on rebase.
  • --destination auto: I've got reservations about the discoverability of this, and we don't currently have a good story for a revset alias of sorts that changes behavior depending on the context in which it's used.
  • A "rebase presets" feature, where we could use the currently-an-error positional argument space for the rebase command to define presets, so that users could define a 'linearize' preset that specifies --revision='orphan()-obsolete()' and --dest=_destautoorphanrebase(SRC).

Personally, I find the third option somewhat appealing, but am
hesitant to "spend" the functionality space of positional arguments to
the rebase command. We should revisit the way we expose this
functionality sometime in the 4.7 cycle once we've had a chance to vet
the implementation of the functionality.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Mar 4 2018, 3:37 PM
durin42 updated this revision to Diff 6611.Mar 4 2018, 3:39 PM
indygreg requested changes to this revision.Mar 4 2018, 5:08 PM
indygreg added a subscriber: indygreg.

I'm +1 on the feature. We should bikeshed the naming.

hgext/rebase.py
123

Let's add this to destutil.py instead of adding more code to rebase.py.

147

This should use the node.nullrev constant.

702

The fact that auto takes an argument makes it... not very auto.

I have a slight preference for `--orphans <rev>`. All the other arguments that take revsets are nouns describing what will be operated on, what the revisions are being used for, etc.

837

An allow list is better. But I could let this slide.

This revision now requires changes to proceed.Mar 4 2018, 5:08 PM
quark added a subscriber: quark.Mar 4 2018, 11:37 PM

My initial idea was to allow -r 'orphan()-obsolete()' -d auto. That can be actually done without any code change by defining auto as a complex revset.

That idea derived an idea of allowing omitting arguments.

rebase.autodest = ... # when -r or -s is provided
rebase.autosrc = ...  # when -d is provided
rebase.autosrcdest = ..., ... # when nothing is provided

So people can run hg rebase, hg rebase -r orphan()-obsolete().

I have also thought about "rebase presets", like:

rebase.preset.linearize.src = orphan() - obsolete()
rebase.preset.linearize.dest = (a complex revset using SRC, ALLSRC)
rebase.preset.master.src = ". % @"
rebase.preset.master.dest = @

If preset name is the unnamed argument, then people can use rebase master, rebase linearize.

durin42 marked an inline comment as done.Apr 9 2018, 4:27 PM
In D2668#43338, @quark wrote:

My initial idea was to allow -r 'orphan()-obsolete()' -d auto. That can be actually done without any code change by defining auto as a complex revset.
That idea derived an idea of allowing omitting arguments.

This is all very cool, but I worry about how we define auto as a rebase-only scoped revset. How do we make that discoverable?

I like the way --auto (or some other name) is discoverable in hg help rebase. Losing that inside the long-form prose of the help text (which is the only place I can think of to document a magic rebase-specific revset?) seems like a bummer to me...

hgext/rebase.py
702

I don't follow. It automatically rebases the specified set of revisions - that is, it automatically picks the destination. Does that make sense?

(I don't know what you think could be more auto here?)

I honestly find it very confusing to write hg rebase --orphans 'orphan()'. Any thoughts about that?

durin42 updated this revision to Diff 7914.Apr 9 2018, 4:34 PM

I like the way --auto (or some other name) is discoverable in hg help rebase. Losing that inside the long-form prose of the help text (which is the only place I can think of to document a magic rebase-specific revset?) seems like a bummer to me...

@spectral had the idea of making --auto imply a default of -r orphan(), but you could override -r if you want. That'd make hg evolve --any --all become hg rebase --auto, if people like that more.

quark added a subscriber: mbthomas.EditedApr 9 2018, 7:29 PM

I like the way --auto (or some other name) is discoverable in hg help rebase. Losing that inside the long-form prose of the help text (which is the only place I can think of to document a magic rebase-specific revset?) seems like a bummer to me...

We recently had some internal discussion about discovery. @mbthomas mentioned that help text is a way of discovery and help text is not only about command line flags. i.e. there could be some examples in help text. I think that might be actually better not only because it keeps CLI clean, but also because a flag has limited (half of a line) space to explain things while an example could have multi-line explanation.

UPDATE: For the revset namespace issue, I think we can define something like rebase.defaultdest, rebase.auto and modify revset resolution layer to think about namespaces. And then pass namespaces to scmutil.rev* from the rebase command.

durin42 edited the summary of this revision. (Show Details)Apr 16 2018, 10:00 PM
durin42 updated this revision to Diff 8361.
krbullock requested changes to this revision.Apr 18 2018, 2:40 PM
krbullock added a subscriber: krbullock.

Looks like a couple unresolved comments on _possibledestination, plus one nitpick. I think we can clean those things up easily and then land this though.

hgext/rebase.py
840

Would be nice to see test coverage of this... it looks to me like we could end up spitting out an error like incompatible with --foo_bar (with an underscore) accidentally. That isn't itself a problem now, but could be in the future. We should at least test the mutual exclusion with other options.

This revision now requires changes to proceed.Apr 18 2018, 2:40 PM
durin42 updated this revision to Diff 8390.Apr 18 2018, 2:50 PM
durin42 marked an inline comment as done.Apr 18 2018, 2:50 PM

I'm not in love with the function name in destutil, but it's the best I've got. Open to suggestions, or we can change it in the future.

krbullock accepted this revision.Apr 18 2018, 3:00 PM
This revision was automatically updated to reflect the committed changes.