Patchwork [1,of,2] rebase: 'hg pull --rebase' now update only if there was nothing to rebase

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 14, 2016, 1:15 a.m.
Message ID <0842ac1829bbc270e9e7.1455412532@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/13171/
State Accepted
Headers show

Comments

Pierre-Yves David - Feb. 14, 2016, 1:15 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1455382772 0
#      Sat Feb 13 16:59:32 2016 +0000
# Node ID 0842ac1829bbc270e9e73cce5ccc2781d25e19a8
# Parent  420968ffb55af1f9182f86da25793920c934c208
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 0842ac1829bb
rebase: 'hg pull --rebase' now update only if there was nothing to rebase

I recently discovered that 'hg pull --rebase' was also running an update. And it
was running it in all cases as long as the update would move the working copy
somewhere else...

This felt wrong and it actually is. This 'update' call is introduced in
92455c1d6f83. In that commit the intent is very clear. The update should happen
only when there was nothing to rebase. The implementation did not check if a
rebase was performed because, at that time, rebase would always leave you on the
top most changeset. Being on that top most changeset result in a no-op update
and the step was skipped.

However 9c78ed396075f changed rebase behavior to preserve the working copy
parent, so if we are not on a head at pull time, the code performs both a rebase
and an update.

This changeset introduce a test for this case and restore the intended behavior.

There are other issues with this custom update code but they will be addressed
in later changeset (eg: own destination logic, lack of heads warning).

I'm not super happy with the explicitly comparison 'rebase(...) == 1' but a
later series will have a cleaner way to handle it anyway (while making 'rebase'
pick its default destination like 'merge').
Martin von Zweigbergk - Feb. 14, 2016, 7:06 a.m.
On Sat, Feb 13, 2016 at 5:15 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1455382772 0
> #      Sat Feb 13 16:59:32 2016 +0000
> # Node ID 0842ac1829bbc270e9e73cce5ccc2781d25e19a8
> # Parent  420968ffb55af1f9182f86da25793920c934c208
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 0842ac1829bb
> rebase: 'hg pull --rebase' now update only if there was nothing to rebase
>
> I recently discovered that 'hg pull --rebase' was also running an update. And it
> was running it in all cases as long as the update would move the working copy
> somewhere else...
>
> This felt wrong and it actually is. This 'update' call is introduced in
> 92455c1d6f83. In that commit the intent is very clear. The update should happen
> only when there was nothing to rebase. The implementation did not check if a
> rebase was performed because, at that time, rebase would always leave you on the
> top most changeset. Being on that top most changeset result in a no-op update
> and the step was skipped.
>
> However 9c78ed396075f changed rebase behavior to preserve the working copy
> parent, so if we are not on a head at pull time, the code performs both a rebase
> and an update.
>
> This changeset introduce a test for this case and restore the intended behavior.
>
> There are other issues with this custom update code but they will be addressed
> in later changeset (eg: own destination logic, lack of heads warning).
>
> I'm not super happy with the explicitly comparison 'rebase(...) == 1'

We can at least spell it "== nothingtorebase()". I'll change that in
flight. I'll drop the then-unnecessary comment below as well. And I'll
drop the Yoda notation (we don't normally use that and I see no reason
to do it here).

> but a
> later series will have a cleaner way to handle it anyway (while making 'rebase'
> pick its default destination like 'merge').
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -1163,19 +1163,21 @@ def pullrebase(orig, ui, repo, *args, **
>                      del opts['rev']
>                  # positional argument from pull conflicts with rebase's own
>                  # --source.
>                  if 'source' in opts:
>                      del opts['source']
> -                rebase(ui, repo, **opts)
> -                branch = repo[None].branch()
> -                dest = repo[branch].rev()
> -                if dest != repo['.'].rev():
> -                    # there was nothing to rebase we force an update
> -                    hg.update(repo, dest)
> -                    if bookmarks.update(repo, [movemarkfrom], repo['.'].node()):
> -                        ui.status(_("updating bookmark %s\n")
> -                                  % repo._activebookmark)
> +                if 1 == rebase(ui, repo, **opts):
> +                    # nothing to rebase
> +                    branch = repo[None].branch()
> +                    dest = repo[branch].rev()
> +                    if dest != repo['.'].rev():
> +                        # there was nothing to rebase we force an update
> +                        hg.update(repo, dest)
> +                        if bookmarks.update(repo, [movemarkfrom],
> +                                            repo['.'].node()):
> +                            ui.status(_("updating bookmark %s\n")
> +                                      % repo._activebookmark)
>          finally:
>              release(lock, wlock)
>      else:
>          if opts.get('tool'):
>              raise error.Abort(_('--tool can only be used with --rebase'))
> diff --git a/tests/test-rebase-pull.t b/tests/test-rebase-pull.t
> --- a/tests/test-rebase-pull.t
> +++ b/tests/test-rebase-pull.t
> @@ -207,5 +207,62 @@ pull --rebase works with bundle2 turned
>    |
>    o  1: 'C2'
>    |
>    o  0: 'C1'
>
> +
> +pull --rebase only update if there is nothing to rebase
> +
> +  $ cd ../a
> +  $ echo R5 > R5
> +  $ hg ci -Am R5
> +  adding R5
> +  $ hg tglog
> +  @  6: 'R5'
> +  |
> +  o  5: 'R4'
> +  |
> +  o  4: 'R3'
> +  |
> +  o  3: 'R2'
> +  |
> +  o  2: 'R1'
> +  |
> +  o  1: 'C2'
> +  |
> +  o  0: 'C1'
> +
> +  $ cd ../c
> +  $ echo L2 > L2
> +  $ hg ci -Am L2
> +  adding L2
> +  $ hg up 'desc(L1)'
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ hg pull --rebase
> +  pulling from $TESTTMP/a (glob)
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files (+1 heads)
> +  rebasing 6:0d0727eb7ce0 "L1"
> +  rebasing 7:c1f58876e3bf "L2"
> +  saved backup bundle to $TESTTMP/c/.hg/strip-backup/0d0727eb7ce0-ef61ccb2-backup.hg (glob)
> +  $ hg tglog
> +  o  8: 'L2'
> +  |
> +  @  7: 'L1'
> +  |
> +  o  6: 'R5'
> +  |
> +  o  5: 'R4'
> +  |
> +  o  4: 'R3'
> +  |
> +  o  3: 'R2'
> +  |
> +  o  2: 'R1'
> +  |
> +  o  1: 'C2'
> +  |
> +  o  0: 'C1'
> +
> _______________________________________________
> 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
@@ -1163,19 +1163,21 @@  def pullrebase(orig, ui, repo, *args, **
                     del opts['rev']
                 # positional argument from pull conflicts with rebase's own
                 # --source.
                 if 'source' in opts:
                     del opts['source']
-                rebase(ui, repo, **opts)
-                branch = repo[None].branch()
-                dest = repo[branch].rev()
-                if dest != repo['.'].rev():
-                    # there was nothing to rebase we force an update
-                    hg.update(repo, dest)
-                    if bookmarks.update(repo, [movemarkfrom], repo['.'].node()):
-                        ui.status(_("updating bookmark %s\n")
-                                  % repo._activebookmark)
+                if 1 == rebase(ui, repo, **opts):
+                    # nothing to rebase
+                    branch = repo[None].branch()
+                    dest = repo[branch].rev()
+                    if dest != repo['.'].rev():
+                        # there was nothing to rebase we force an update
+                        hg.update(repo, dest)
+                        if bookmarks.update(repo, [movemarkfrom],
+                                            repo['.'].node()):
+                            ui.status(_("updating bookmark %s\n")
+                                      % repo._activebookmark)
         finally:
             release(lock, wlock)
     else:
         if opts.get('tool'):
             raise error.Abort(_('--tool can only be used with --rebase'))
diff --git a/tests/test-rebase-pull.t b/tests/test-rebase-pull.t
--- a/tests/test-rebase-pull.t
+++ b/tests/test-rebase-pull.t
@@ -207,5 +207,62 @@  pull --rebase works with bundle2 turned 
   |
   o  1: 'C2'
   |
   o  0: 'C1'
   
+
+pull --rebase only update if there is nothing to rebase
+
+  $ cd ../a
+  $ echo R5 > R5
+  $ hg ci -Am R5
+  adding R5
+  $ hg tglog
+  @  6: 'R5'
+  |
+  o  5: 'R4'
+  |
+  o  4: 'R3'
+  |
+  o  3: 'R2'
+  |
+  o  2: 'R1'
+  |
+  o  1: 'C2'
+  |
+  o  0: 'C1'
+  
+  $ cd ../c
+  $ echo L2 > L2
+  $ hg ci -Am L2
+  adding L2
+  $ hg up 'desc(L1)'
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg pull --rebase
+  pulling from $TESTTMP/a (glob)
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files (+1 heads)
+  rebasing 6:0d0727eb7ce0 "L1"
+  rebasing 7:c1f58876e3bf "L2"
+  saved backup bundle to $TESTTMP/c/.hg/strip-backup/0d0727eb7ce0-ef61ccb2-backup.hg (glob)
+  $ hg tglog
+  o  8: 'L2'
+  |
+  @  7: 'L1'
+  |
+  o  6: 'R5'
+  |
+  o  5: 'R4'
+  |
+  o  4: 'R3'
+  |
+  o  3: 'R2'
+  |
+  o  2: 'R1'
+  |
+  o  1: 'C2'
+  |
+  o  0: 'C1'
+