Patchwork context: explicitly return a tuple

login
register
mail settings
Submitter Sean Farley
Date May 28, 2014, 5:48 p.m.
Message ID <ebae0401cb6b85442061.1401299307@laptop.local>
Download mbox | patch
Permalink /patch/4893/
State Accepted
Commit 0a8e7f81e8ae199d75241755534038f8201ae12c
Headers show

Comments

Sean Farley - May 28, 2014, 5:48 p.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1401228288 18000
#      Tue May 27 17:04:48 2014 -0500
# Node ID ebae0401cb6b854420619962316005e814faad78
# Parent  83bbfb23cb24b473286d528ddccbb333329f7f29
context: explicitly return a tuple

In the refactoring of removing localrepo.status, 2edb8648c500, we accidentally
changed the return type from a tuple to a list. Philosophically, this is
incorrect so we explicitly return a tuple again.
Pierre-Yves David - May 28, 2014, 6:16 p.m.
On 05/28/2014 10:48 AM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1401228288 18000
> #      Tue May 27 17:04:48 2014 -0500
> # Node ID ebae0401cb6b854420619962316005e814faad78
> # Parent  83bbfb23cb24b473286d528ddccbb333329f7f29
> context: explicitly return a tuple
>
> In the refactoring of removing localrepo.status, 2edb8648c500, we accidentally
> changed the return type from a tuple to a list. Philosophically, this is
> incorrect so we explicitly return a tuple again.
>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -324,11 +324,13 @@ class basectx(object):
>                       self._repo.ui.status(_("skipping missing "
>                                              "subrepository: %s\n") % subpath)
>
>           for l in r:
>               l.sort()
> -        return r
> +
> +        # we return a tuple to signify that this list isn't changing
> +        return tuple(r)
>
>
>   def makememctx(repo, parents, text, user, date, branch, files, store,
>                  editor=None):
>       def getfilectx(repo, memctx, path):
> @@ -1444,11 +1446,11 @@ class workingctx(committablectx):
>           # 'memctx'?
>           s = super(workingctx, self).status(other, match, listignored, listclean,
>                                              listunknown, listsubrepos)
>           # calling 'super' subtly reveresed the contexts, so we flip the results
>           # (s[1] is 'added' and s[2] is 'removed')
> -        s[1], s[2] = s[2], s[1]
> +        s = s[2], s[1], s[3], s[4]

Isn't this going to have awful silent consequences when 5 < len(s) or 
when someone write code under alcohol influence and silently drop the 
[0] item?
Sean Farley - May 28, 2014, 6:32 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 05/28/2014 10:48 AM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1401228288 18000
>> #      Tue May 27 17:04:48 2014 -0500
>> # Node ID ebae0401cb6b854420619962316005e814faad78
>> # Parent  83bbfb23cb24b473286d528ddccbb333329f7f29
>> context: explicitly return a tuple
>>
>> In the refactoring of removing localrepo.status, 2edb8648c500, we accidentally
>> changed the return type from a tuple to a list. Philosophically, this is
>> incorrect so we explicitly return a tuple again.
>>
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -324,11 +324,13 @@ class basectx(object):
>>                       self._repo.ui.status(_("skipping missing "
>>                                              "subrepository: %s\n") % subpath)
>>
>>           for l in r:
>>               l.sort()
>> -        return r
>> +
>> +        # we return a tuple to signify that this list isn't changing
>> +        return tuple(r)
>>
>>
>>   def makememctx(repo, parents, text, user, date, branch, files, store,
>>                  editor=None):
>>       def getfilectx(repo, memctx, path):
>> @@ -1444,11 +1446,11 @@ class workingctx(committablectx):
>>           # 'memctx'?
>>           s = super(workingctx, self).status(other, match, listignored, listclean,
>>                                              listunknown, listsubrepos)
>>           # calling 'super' subtly reveresed the contexts, so we flip the results
>>           # (s[1] is 'added' and s[2] is 'removed')
>> -        s[1], s[2] = s[2], s[1]
>> +        s = s[2], s[1], s[3], s[4]
>
> Isn't this going to have awful silent consequences when 5 < len(s) or 
> when someone write code under alcohol influence and silently drop the 
> [0] item?

Ugh, this was a bad overlook on my part. I'll fix it up.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -324,11 +324,13 @@  class basectx(object):
                     self._repo.ui.status(_("skipping missing "
                                            "subrepository: %s\n") % subpath)
 
         for l in r:
             l.sort()
-        return r
+
+        # we return a tuple to signify that this list isn't changing
+        return tuple(r)
 
 
 def makememctx(repo, parents, text, user, date, branch, files, store,
                editor=None):
     def getfilectx(repo, memctx, path):
@@ -1444,11 +1446,11 @@  class workingctx(committablectx):
         # 'memctx'?
         s = super(workingctx, self).status(other, match, listignored, listclean,
                                            listunknown, listsubrepos)
         # calling 'super' subtly reveresed the contexts, so we flip the results
         # (s[1] is 'added' and s[2] is 'removed')
-        s[1], s[2] = s[2], s[1]
+        s = s[2], s[1], s[3], s[4]
         return s
 
 class committablefilectx(basefilectx):
     """A committablefilectx provides common functionality for a file context
     that wants the ability to commit, e.g. workingfilectx or memfilectx."""