Patchwork [remotenames] pull: fix pull --rebase with a --tool argument

login
register
mail settings
Submitter Adam Simpkins
Date May 30, 2017, 7:47 p.m.
Message ID <3988f1c49f0d86d6e0c8.1496173635@devbig125.prn1.facebook.com>
Download mbox | patch
Permalink /patch/21074/
State Accepted
Headers show

Comments

Adam Simpkins - May 30, 2017, 7:47 p.m.
# HG changeset patch
# User Adam Simpkins <simpkins@fb.com>
# Date 1496173480 25200
#      Tue May 30 12:44:40 2017 -0700
# Node ID 3988f1c49f0d86d6e0c8a7bbfaf738d70fddf916
# Parent  78c8966c02759755235a6566e0c557b82ff9cb0a
pull: fix pull --rebase with a --tool argument

The code previously stripped off the --rebase argument when invoking the
original pull() command code, but did not strip off --tool, causing the command
to incorrectly fail.

This fixes the code to also strip off --tool when invoking pull, and then pass
this argument in when doing the rebase.

However, the way the rebase code invokes merge does seem to result in rather
confusing behavior: the "internal:other" tool preserves the local changes,
while "internal:local" preserves the changes pulled from the remote.
Sean Farley - May 31, 2017, 3:34 a.m.
Adam Simpkins <simpkins@fb.com> writes:

> # HG changeset patch
> # User Adam Simpkins <simpkins@fb.com>
> # Date 1496173480 25200
> #      Tue May 30 12:44:40 2017 -0700
> # Node ID 3988f1c49f0d86d6e0c8a7bbfaf738d70fddf916
> # Parent  78c8966c02759755235a6566e0c557b82ff9cb0a
> pull: fix pull --rebase with a --tool argument
>
> The code previously stripped off the --rebase argument when invoking the
> original pull() command code, but did not strip off --tool, causing the command
> to incorrectly fail.
>
> This fixes the code to also strip off --tool when invoking pull, and then pass
> this argument in when doing the rebase.

Ah, good catch; queued!

> However, the way the rebase code invokes merge does seem to result in rather
> confusing behavior: the "internal:other" tool preserves the local changes,
> while "internal:local" preserves the changes pulled from the remote.

Isn't this the case with all rebases (it switches the sides)?
Sean Farley - May 31, 2017, 3:39 a.m.
Adam Simpkins <simpkins@fb.com> writes:

> # HG changeset patch
> # User Adam Simpkins <simpkins@fb.com>
> # Date 1496173480 25200
> #      Tue May 30 12:44:40 2017 -0700
> # Node ID 3988f1c49f0d86d6e0c8a7bbfaf738d70fddf916
> # Parent  78c8966c02759755235a6566e0c557b82ff9cb0a
> pull: fix pull --rebase with a --tool argument
>
> The code previously stripped off the --rebase argument when invoking the
> original pull() command code, but did not strip off --tool, causing the command
> to incorrectly fail.
>
> This fixes the code to also strip off --tool when invoking pull, and then pass
> this argument in when doing the rebase.
>
> However, the way the rebase code invokes merge does seem to result in rather
> confusing behavior: the "internal:other" tool preserves the local changes,
> while "internal:local" preserves the changes pulled from the remote.
>
> diff --git a/remotenames.py b/remotenames.py
> --- a/remotenames.py
> +++ b/remotenames.py
> @@ -880,8 +880,9 @@
>      if dest:
>          # Let `pull` do its thing without `rebase.py->pullrebase()`
>          del opts['rebase']
> +        tool = opts.pop('tool', '')
>          ret = orig(ui, repo, source, **opts)
> -        return ret or rebasemodule.rebase(ui, repo, dest=dest)
> +        return ret or rebasemodule.rebase(ui, repo, dest=dest, tool=tool)
>      else:
>          return orig(ui, repo, source, **opts)
>  
> diff --git a/tests/test-pull-rebase.t b/tests/test-pull-rebase.t
> --- a/tests/test-pull-rebase.t
> +++ b/tests/test-pull-rebase.t
> @@ -151,3 +151,62 @@
>    $ hg -q pull
>    $ hg log -l 1 --template="{desc} {remotenames}\n"
>    between_pull default/bookmarkonremote default/default
> +
> +Test pull with --rebase and --tool
> +  $ cd ../remoterepo
> +  $ hg up bookmarkonremote -q
> +  $ echo remotechanges > editedbyboth
> +  $ hg add editedbyboth
> +  $ mkcommit remotecommit
> +  $ cd ../localrepo
> +  $ hg book -t default/bookmarkonremote -r default/bookmarkonremote tracking2
> +  $ hg update tracking2 -q
> +  $ echo localchanges > editedbyboth
> +  $ hg add editedbyboth
> +  $ mkcommit somelocalchanges
> +  $ printdag
> +  @  somelocalchanges | tracking2 |
> +  |
> +  o  between_pull |  | default/bookmarkonremote
> +  |
> +  o  foo |  |
> +  |
> +  | o  localcommit | bmnottracking tracking |
> +  | |
> +  | o  untrackedremotecommit |  |
> +  | |
> +  o |  trackedremotecommit |  |
> +  |/
> +  o  root |  |
> +  
> +  $ hg pull --rebase --tool internal:union
> +  pulling from $TESTTMP/remoterepo

Forgot a (glob) here:

   $ hg locate | sed 's-\\-/-g' |
   >   xargs "$check_code" --warnings --per-file=0 || false
+  tests/test-pull-rebase.t:183:
+   >   pulling from $TESTTMP/remoterepo
+   use (glob) to match Windows paths too
+  [1]

test-check-code-hg.t says "hi" ;-)

(fixed in-flight)

Patch

diff --git a/remotenames.py b/remotenames.py
--- a/remotenames.py
+++ b/remotenames.py
@@ -880,8 +880,9 @@ 
     if dest:
         # Let `pull` do its thing without `rebase.py->pullrebase()`
         del opts['rebase']
+        tool = opts.pop('tool', '')
         ret = orig(ui, repo, source, **opts)
-        return ret or rebasemodule.rebase(ui, repo, dest=dest)
+        return ret or rebasemodule.rebase(ui, repo, dest=dest, tool=tool)
     else:
         return orig(ui, repo, source, **opts)
 
diff --git a/tests/test-pull-rebase.t b/tests/test-pull-rebase.t
--- a/tests/test-pull-rebase.t
+++ b/tests/test-pull-rebase.t
@@ -151,3 +151,62 @@ 
   $ hg -q pull
   $ hg log -l 1 --template="{desc} {remotenames}\n"
   between_pull default/bookmarkonremote default/default
+
+Test pull with --rebase and --tool
+  $ cd ../remoterepo
+  $ hg up bookmarkonremote -q
+  $ echo remotechanges > editedbyboth
+  $ hg add editedbyboth
+  $ mkcommit remotecommit
+  $ cd ../localrepo
+  $ hg book -t default/bookmarkonremote -r default/bookmarkonremote tracking2
+  $ hg update tracking2 -q
+  $ echo localchanges > editedbyboth
+  $ hg add editedbyboth
+  $ mkcommit somelocalchanges
+  $ printdag
+  @  somelocalchanges | tracking2 |
+  |
+  o  between_pull |  | default/bookmarkonremote
+  |
+  o  foo |  |
+  |
+  | o  localcommit | bmnottracking tracking |
+  | |
+  | o  untrackedremotecommit |  |
+  | |
+  o |  trackedremotecommit |  |
+  |/
+  o  root |  |
+  
+  $ hg pull --rebase --tool internal:union
+  pulling from $TESTTMP/remoterepo
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 2 changes to 2 files (+1 heads)
+  (run 'hg heads .' to see heads, 'hg merge' to merge)
+  rebasing 6:1d01e32a0efb "somelocalchanges" (tracking2)
+  merging editedbyboth
+  saved backup bundle to $TESTTMP/localrepo/.hg/strip-backup/*-backup.hg (glob)
+  $ printdag
+  @  somelocalchanges | tracking2 |
+  |
+  o  remotecommit |  | default/bookmarkonremote
+  |
+  o  between_pull |  |
+  |
+  o  foo |  |
+  |
+  | o  localcommit | bmnottracking tracking |
+  | |
+  | o  untrackedremotecommit |  |
+  | |
+  o |  trackedremotecommit |  |
+  |/
+  o  root |  |
+  
+  $ cat editedbyboth
+  remotechanges
+  localchanges