Patchwork [RESEND] histedit: select the lowest rev when looking for a root in a revset

login
register
mail settings
Submitter David Soria Parra
Date March 17, 2014, 5:10 p.m.
Message ID <882b4e7f82a01f62c1ae.1395076205@dev544.prn1.facebook.com>
Download mbox | patch
Permalink /patch/3956/
State Superseded
Headers show

Comments

David Soria Parra - March 17, 2014, 5:10 p.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1394751906 25200
#      Thu Mar 13 16:05:06 2014 -0700
# Node ID 882b4e7f82a01f62c1ae23e0700a2a23d35498d1
# Parent  c152e538b85b099ce20b51104b8b7dd3666aad7c
histedit: select the lowest rev when looking for a root in a revset

When we specify a revision or a revset we just get the last element from the
list. For revsets this can lead to unintended effects where you specify a
revset like only() but instead histedit selects the highest revision in the
set as root. Therefore we should always use the lowest revision number as
root.
Siddharth Agarwal - March 18, 2014, 12:13 a.m.
On 03/17/2014 10:10 AM, David Soria Parra wrote:
> +            root = 'min(%s)' % revs[0]
>               root = scmutil.revsingle(repo, root).node()

Can you actually interpolate revs[0] like that?
Siddharth Agarwal - March 18, 2014, 12:21 a.m.
On 03/17/2014 05:13 PM, Siddharth Agarwal wrote:
> On 03/17/2014 10:10 AM, David Soria Parra wrote:
>> +            root = 'min(%s)' % revs[0]
>>               root = scmutil.revsingle(repo, root).node()
>
> Can you actually interpolate revs[0] like that?

I meant in terms of needing to escape revs[0], etc. You might need to 
use repo.revs() here. I don't know how you'd resolve backwards 
compatibility: scmutil.revrange(), which revsingle() calls, has the 
notion of old-style queries.
David Soria Parra - March 18, 2014, 4:06 p.m.
On 3/17/14, 5:21 PM, "Siddharth Agarwal" <sid0@fb.com> wrote:

>On 03/17/2014 05:13 PM, Siddharth Agarwal wrote:
>> On 03/17/2014 10:10 AM, David Soria Parra wrote:
>>> +            root = 'min(%s)' % revs[0]
>>>               root = scmutil.revsingle(repo, root).node()
>>
>> Can you actually interpolate revs[0] like that?
>
>I meant in terms of needing to escape revs[0], etc. You might need to
>use repo.revs() here. I don't know how you'd resolve backwards
>compatibility: scmutil.revrange(), which revsingle() calls, has the
>notion of old-style queries.

rev single uses scmutil.revrange which we usually use to do parse the
non-escaped revisions, so therefore my understanding is that it¹s fine to
do that
this way.
Augie Fackler - March 18, 2014, 4:14 p.m.
Looks totally reasonable, but please write a test before I accept this.


On Mon, Mar 17, 2014 at 1:10 PM, David Soria Parra <davidsp@fb.com> wrote:

> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1394751906 25200
> #      Thu Mar 13 16:05:06 2014 -0700
> # Node ID 882b4e7f82a01f62c1ae23e0700a2a23d35498d1
> # Parent  c152e538b85b099ce20b51104b8b7dd3666aad7c
> histedit: select the lowest rev when looking for a root in a revset
>
> When we specify a revision or a revset we just get the last element from
> the
> list. For revsets this can lead to unintended effects where you specify a
> revset like only() but instead histedit selects the highest revision in the
> set as root. Therefore we should always use the lowest revision number as
> root.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -565,7 +565,7 @@
>                  remote = None
>              root = findoutgoing(ui, repo, remote, force, opts)
>          else:
> -            root = revs[0]
> +            root = 'min(%s)' % revs[0]
>              root = scmutil.revsingle(repo, root).node()
>
>          keep = opts.get('keep', False)
>
Pierre-Yves David - March 18, 2014, 7:42 p.m.
On 03/17/2014 10:10 AM, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1394751906 25200
> #      Thu Mar 13 16:05:06 2014 -0700
> # Node ID 882b4e7f82a01f62c1ae23e0700a2a23d35498d1
> # Parent  c152e538b85b099ce20b51104b8b7dd3666aad7c
> histedit: select the lowest rev when looking for a root in a revset
>
> When we specify a revision or a revset we just get the last element from the
> list. For revsets this can lead to unintended effects where you specify a
> revset like only() but instead histedit selects the highest revision in the
> set as root. Therefore we should always use the lowest revision number as
> root.

Sounds like  you are fixing a limit of the current implementation. But 
youa re not adding any new test :-p

> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -565,7 +565,7 @@
>                   remote = None
>               root = findoutgoing(ui, repo, remote, force, opts)
>           else:
> -            root = revs[0]
> +            root = 'min(%s)' % revs[0]

This is not going to fly is the rev is not revset compabible.

I would call something like revrange then process the result to make 
sure there is a single root and use that.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -565,7 +565,7 @@ 
                 remote = None
             root = findoutgoing(ui, repo, remote, force, opts)
         else:
-            root = revs[0]
+            root = 'min(%s)' % revs[0]
             root = scmutil.revsingle(repo, root).node()
 
         keep = opts.get('keep', False)