Patchwork rebase: split _checkobsrebase into two parts for better wrapping

login
register
mail settings
Submitter Kostia Balytskyi
Date May 30, 2016, 5:31 p.m.
Message ID <330fa585c8b9a272f51e.1464629480@ikostia-mbp>
Download mbox | patch
Permalink /patch/15265/
State Deferred
Headers show

Comments

Kostia Balytskyi - May 30, 2016, 5:31 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1464629336 -3600
#      Mon May 30 18:28:56 2016 +0100
# Node ID 330fa585c8b9a272f51e6b82a8f65d9b01e0207e
# Parent  318534bb5dfd37b1c95f10ba90b73c5ce562713f
rebase: split _checkobsrebase into two parts for better wrapping

Currently _checkobsrebase does two checks: one aborts if divergence is going
to be caused by the rebase operation (and this can be turned off by a config),
another aborts if all changesets have successors in the destination (and this
cannot be turned off). I have suspicions that some rebase users would like it
not to abort when there's nothing to rebase, but rather to exit with code 0.
Splitting _checkobsrebase is a step in this direction.

This is unrelated to the other refactoring series.
Pierre-Yves David - June 2, 2016, 1:25 a.m.
On 05/30/2016 07:31 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1464629336 -3600
> #      Mon May 30 18:28:56 2016 +0100
> # Node ID 330fa585c8b9a272f51e6b82a8f65d9b01e0207e
> # Parent  318534bb5dfd37b1c95f10ba90b73c5ce562713f
> rebase: split _checkobsrebase into two parts for better wrapping
> 
> Currently _checkobsrebase does two checks: one aborts if divergence is going
> to be caused by the rebase operation (and this can be turned off by a config),
> another aborts if all changesets have successors in the destination (and this
> cannot be turned off). I have suspicions that some rebase users would like it
> not to abort when there's nothing to rebase, but rather to exit with code 0.
> Splitting _checkobsrebase is a step in this direction.
> 
> This is unrelated to the other refactoring series.

I actually lean toward have that alternate behavior as the default, I
can't remember the discussion with Laurent that led to the current
default (if we had one), but the current abort feel strange to me and I
would rather see the rebase proceed successfully (it will have to update
the working copy parent and related bookmarks)

I've CC Laurent who might have valuable insight on why the current
behavior was picked.
Augie Fackler - June 4, 2016, 2:39 a.m.
On Thu, Jun 02, 2016 at 03:25:58AM +0200, Pierre-Yves David wrote:
>
>
> On 05/30/2016 07:31 PM, Kostia Balytskyi wrote:
> > # HG changeset patch
> > # User Kostia Balytskyi <ikostia@fb.com>
> > # Date 1464629336 -3600
> > #      Mon May 30 18:28:56 2016 +0100
> > # Node ID 330fa585c8b9a272f51e6b82a8f65d9b01e0207e
> > # Parent  318534bb5dfd37b1c95f10ba90b73c5ce562713f
> > rebase: split _checkobsrebase into two parts for better wrapping
> >
> > Currently _checkobsrebase does two checks: one aborts if divergence is going
> > to be caused by the rebase operation (and this can be turned off by a config),
> > another aborts if all changesets have successors in the destination (and this
> > cannot be turned off). I have suspicions that some rebase users would like it
> > not to abort when there's nothing to rebase, but rather to exit with code 0.
> > Splitting _checkobsrebase is a step in this direction.
> >
> > This is unrelated to the other refactoring series.
>
> I actually lean toward have that alternate behavior as the default, I
> can't remember the discussion with Laurent that led to the current
> default (if we had one), but the current abort feel strange to me and I
> would rather see the rebase proceed successfully (it will have to update
> the working copy parent and related bookmarks)

I'm agreed on the current behavior as a bug, for what it's worth. It's
a deviation from rebase-without-obsolete, and I've found it surprising
more than once as a long-time rebase enthusiast.

>
> I've CC Laurent who might have valuable insight on why the current
> behavior was picked.
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -737,6 +737,33 @@ 
     else:
         return None
 
+def _abortifdivergencecaused(repo, ui, rebaseobsrevs, rebaseobsskipped):
+    """Abort if rebasing will cuase divergence"""
+    # Obsolete node with successors not in dest leads to divergence
+    divergenceok = ui.configbool('experimental', 'allowdivergence')
+    divergencebasecandidates = rebaseobsrevs - rebaseobsskipped
+
+    if divergencebasecandidates and not divergenceok:
+        divhashes = (str(repo[r]) for r in divergencebasecandidates)
+        msg = _("this rebase will cause "
+                "divergences from: %s")
+        h = _("to force the rebase please set "
+              "experimental.allowdivergence=True")
+        raise error.Abort(msg % (",".join(divhashes),), hint=h)
+
+def _abortifnothingtorebase(repo, ui, rebasesetrevs, rebaseobsskipped):
+    """Abort if every changeset has a successor in the destination"""
+    # - plain prune (no successor) changesets are rebased
+    # - split changesets are not rebased if at least one of the
+    # changeset resulting from the split is an ancestor of dest
+    rebaseset = rebasesetrevs - rebaseobsskipped
+    if rebasesetrevs and not rebaseset:
+        msg = _('all requested changesets have equivalents '
+                'or were marked as obsolete')
+        hint = _('to force the rebase, set the config '
+                 'experimental.rebaseskipobsolete to False')
+        raise error.Abort(msg, hint=hint)
+
 def _checkobsrebase(repo, ui,
                                   rebaseobsrevs,
                                   rebasesetrevs,
@@ -749,30 +776,8 @@ 
     `rebaseobsskipped`: set of revisions from source skipped because they have
     successors in destination
     """
-    # Obsolete node with successors not in dest leads to divergence
-    divergenceok = ui.configbool('experimental',
-                                 'allowdivergence')
-    divergencebasecandidates = rebaseobsrevs - rebaseobsskipped
-
-    if divergencebasecandidates and not divergenceok:
-        divhashes = (str(repo[r])
-                     for r in divergencebasecandidates)
-        msg = _("this rebase will cause "
-                "divergences from: %s")
-        h = _("to force the rebase please set "
-              "experimental.allowdivergence=True")
-        raise error.Abort(msg % (",".join(divhashes),), hint=h)
-
-    # - plain prune (no successor) changesets are rebased
-    # - split changesets are not rebased if at least one of the
-    # changeset resulting from the split is an ancestor of dest
-    rebaseset = rebasesetrevs - rebaseobsskipped
-    if rebasesetrevs and not rebaseset:
-        msg = _('all requested changesets have equivalents '
-                'or were marked as obsolete')
-        hint = _('to force the rebase, set the config '
-                 'experimental.rebaseskipobsolete to False')
-        raise error.Abort(msg, hint=hint)
+    _abortifdivergencecaused(repo, ui, rebaseobsrevs, rebaseobsskipped)
+    _abortifnothingtorebase(repo, ui, rebasesetrevs, rebaseobsskipped)
 
 def defineparents(repo, rev, target, state, targetancestors,
                   obsoletenotrebased):