Patchwork [STABLE] rebase: fix rebase with no common ancestors (issue4446)

login
register
mail settings
Submitter Durham Goode
Date Nov. 10, 2014, 6:46 p.m.
Message ID <15e6fe3cd01beba42327.1415645219@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6665/
State Accepted
Headers show

Comments

Durham Goode - Nov. 10, 2014, 6:46 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1415645082 28800
#      Mon Nov 10 10:44:42 2014 -0800
# Node ID 15e6fe3cd01beba4232780cdef715d47cc0b0df5
# Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
rebase: fix rebase with no common ancestors (issue4446)

The new rebase revset didn't check for the case when there are no common
ancestors. Now it does. The new behavior should be the same as the pre-3.2
behavior. Added a test.
Jordi Gutiérrez Hermoso - Nov. 10, 2014, 7:11 p.m.
On Mon, 2014-11-10 at 10:46 -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1415645082 28800
> #      Mon Nov 10 10:44:42 2014 -0800
> # Node ID 15e6fe3cd01beba4232780cdef715d47cc0b0df5
> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> rebase: fix rebase with no common ancestors (issue4446)

I was working on this, and your fix lgtm. Below I get more picky about
the test you added, but I guess it's ok as it is, if you don't think
it needs anything more.

> --- a/tests/test-rebase-parameters.t
> +++ b/tests/test-rebase-parameters.t
> @@ -468,3 +468,17 @@ Test --tool parameter:
>    [255]
>  
>    $ cd ..
> +
> +No common ancestor
> +
> +  $ hg init separaterepo
> +  $ cd separaterepo
> +  $ touch a
> +  $ hg commit -Aqm a
> +  $ hg up -q null
> +  $ touch b
> +  $ hg commit -Aqm b
> +  $ hg rebase -d 0
> +  nothing to rebase from d7486e00c6f1 to 3903775176ed
> +  [1]
> +  $ cd ..

Should we also test `hg rebase -b` for completeness? Or test anything
that requires more than two commits?
Augie Fackler - Nov. 10, 2014, 7:14 p.m.
On Mon, Nov 10, 2014 at 10:46:59AM -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1415645082 28800
> #      Mon Nov 10 10:44:42 2014 -0800
> # Node ID 15e6fe3cd01beba4232780cdef715d47cc0b0df5
> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> rebase: fix rebase with no common ancestors (issue4446)

Queued, many thanks.

>
> The new rebase revset didn't check for the case when there are no common
> ancestors. Now it does. The new behavior should be the same as the pre-3.2
> behavior. Added a test.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -274,8 +274,12 @@ def rebase(ui, repo, **opts):
>                                  "can't compute rebase set\n"))
>                      return 1
>                  commonanc = repo.revs('ancestor(%ld, %d)', base, dest).first()
> -                rebaseset = repo.revs('(%d::(%ld) - %d)::',
> -                                      commonanc, base, commonanc)
> +                if commonanc is not None:
> +                    rebaseset = repo.revs('(%d::(%ld) - %d)::',
> +                                          commonanc, base, commonanc)
> +                else:
> +                    rebaseset = []
> +
>                  if not rebaseset:
>                      # transform to list because smartsets are not comparable to
>                      # lists. This should be improved to honor laziness of
> diff --git a/tests/test-rebase-parameters.t b/tests/test-rebase-parameters.t
> --- a/tests/test-rebase-parameters.t
> +++ b/tests/test-rebase-parameters.t
> @@ -468,3 +468,17 @@ Test --tool parameter:
>    [255]
>
>    $ cd ..
> +
> +No common ancestor
> +
> +  $ hg init separaterepo
> +  $ cd separaterepo
> +  $ touch a
> +  $ hg commit -Aqm a
> +  $ hg up -q null
> +  $ touch b
> +  $ hg commit -Aqm b
> +  $ hg rebase -d 0
> +  nothing to rebase from d7486e00c6f1 to 3903775176ed
> +  [1]
> +  $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Nov. 10, 2014, 7:16 p.m.
On Mon, Nov 10, 2014 at 2:11 PM, Jordi Gutiérrez Hermoso
<jordigh@octave.org> wrote:
>> rebase: fix rebase with no common ancestors (issue4446)
>
> I was working on this, and your fix lgtm. Below I get more picky about
> the test you added, but I guess it's ok as it is, if you don't think
> it needs anything more.


I'm not /too/ worried about additional tests, but if you have extra
testcases we could test, please mail them for default?
Durham Goode - Nov. 10, 2014, 7:22 p.m.
On 11/10/14 11:11 AM, Jordi Gutiérrez Hermoso wrote:
> On Mon, 2014-11-10 at 10:46 -0800, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1415645082 28800
>> #      Mon Nov 10 10:44:42 2014 -0800
>> # Node ID 15e6fe3cd01beba4232780cdef715d47cc0b0df5
>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>> rebase: fix rebase with no common ancestors (issue4446)
> I was working on this, and your fix lgtm. Below I get more picky about
Sorry, I didn't check the irc to see if anyone was working on it.
> the test you added, but I guess it's ok as it is, if you don't think
> it needs anything more.
>
>> --- a/tests/test-rebase-parameters.t
>> +++ b/tests/test-rebase-parameters.t
>> @@ -468,3 +468,17 @@ Test --tool parameter:
>>     [255]
>>   
>>     $ cd ..
>> +
>> +No common ancestor
>> +
>> +  $ hg init separaterepo
>> +  $ cd separaterepo
>> +  $ touch a
>> +  $ hg commit -Aqm a
>> +  $ hg up -q null
>> +  $ touch b
>> +  $ hg commit -Aqm b
>> +  $ hg rebase -d 0
>> +  nothing to rebase from d7486e00c6f1 to 3903775176ed
>> +  [1]
>> +  $ cd ..
> Should we also test `hg rebase -b` for completeness? Or test anything
> that requires more than two commits?
The code path is covered by this case, so I don't think any other tests 
are necessary unless you can think of similar scenario that could cause 
problems.

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -274,8 +274,12 @@  def rebase(ui, repo, **opts):
                                 "can't compute rebase set\n"))
                     return 1
                 commonanc = repo.revs('ancestor(%ld, %d)', base, dest).first()
-                rebaseset = repo.revs('(%d::(%ld) - %d)::',
-                                      commonanc, base, commonanc)
+                if commonanc is not None:
+                    rebaseset = repo.revs('(%d::(%ld) - %d)::',
+                                          commonanc, base, commonanc)
+                else:
+                    rebaseset = []
+
                 if not rebaseset:
                     # transform to list because smartsets are not comparable to
                     # lists. This should be improved to honor laziness of
diff --git a/tests/test-rebase-parameters.t b/tests/test-rebase-parameters.t
--- a/tests/test-rebase-parameters.t
+++ b/tests/test-rebase-parameters.t
@@ -468,3 +468,17 @@  Test --tool parameter:
   [255]
 
   $ cd ..
+
+No common ancestor
+
+  $ hg init separaterepo
+  $ cd separaterepo
+  $ touch a
+  $ hg commit -Aqm a
+  $ hg up -q null
+  $ touch b
+  $ hg commit -Aqm b
+  $ hg rebase -d 0
+  nothing to rebase from d7486e00c6f1 to 3903775176ed
+  [1]
+  $ cd ..