Patchwork [1,of,9] largefiles: use 'list*' variables for consistency and clarity

login
register
mail settings
Submitter Martin von Zweigbergk
Date Sept. 17, 2014, 8:40 p.m.
Message ID <3eb908098e46eb9b60bf.1410986418@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/5854/
State Changes Requested
Headers show

Comments

Martin von Zweigbergk - Sept. 17, 2014, 8:40 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@gmail.com>
# Date 1410885827 25200
#      Tue Sep 16 09:43:47 2014 -0700
# Node ID 3eb908098e46eb9b60bf6d4cc66a416111f2e201
# Parent  48791c2bea1ceda4e4f28bc11651e281d636ce1a
largefiles: use 'list*' variables for consistency and clarity

The variables 'listignored', 'listclean' and 'listunknown' are
initialized to the values of 'ignored', 'clean' and 'unknown',
respectively. However, the 'ignored', 'clean' and 'unknown' themselves
get a different meaning further down in the method, so use the list*
names for clarity, since they only have one meaning.
Mads Kiilerich - Sept. 20, 2014, 4:31 p.m.
On 09/17/2014 10:40 PM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@gmail.com>
> # Date 1410885827 25200
> #      Tue Sep 16 09:43:47 2014 -0700
> # Node ID 3eb908098e46eb9b60bf6d4cc66a416111f2e201
> # Parent  48791c2bea1ceda4e4f28bc11651e281d636ce1a
> largefiles: use 'list*' variables for consistency and clarity
>
> The variables 'listignored', 'listclean' and 'listunknown' are
> initialized to the values of 'ignored', 'clean' and 'unknown',
> respectively. However, the 'ignored', 'clean' and 'unknown' themselves
> get a different meaning further down in the method, so use the list*
> names for clarity, since they only have one meaning.

AFAICS it is only 'clean' that is used for other purposes later on. (It 
was changed recently when unused values were given a _ prefix.) 'clean' 
starts out as a parameter in a monkey patched wrapper function so it 
should keep its name. I think the real problem is that we reuse 'clean' 
later on - it would be better to fix that and avoid introducing the 
'list*' aliases.

/Mads

>
> diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
> --- a/hgext/largefiles/reposetup.py
> +++ b/hgext/largefiles/reposetup.py
> @@ -152,7 +152,7 @@
>                       m._files = tostandins(m._files)
>   
>                       result = super(lfilesrepo, self).status(node1, node2, m,
> -                        ignored, clean, unknown, listsubrepos)
> +                        listignored, listclean, listunknown, listsubrepos)
>                       if working:
>   
>                           def sfindirstate(f):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Sept. 22, 2014, 4:46 p.m.
On Sat, Sep 20, 2014 at 9:31 AM, Mads Kiilerich <mads@kiilerich.com> wrote:
> On 09/17/2014 10:40 PM, Martin von Zweigbergk wrote:
>>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@gmail.com>
>> # Date 1410885827 25200
>> #      Tue Sep 16 09:43:47 2014 -0700
>> # Node ID 3eb908098e46eb9b60bf6d4cc66a416111f2e201
>> # Parent  48791c2bea1ceda4e4f28bc11651e281d636ce1a
>> largefiles: use 'list*' variables for consistency and clarity
>>
>> The variables 'listignored', 'listclean' and 'listunknown' are
>> initialized to the values of 'ignored', 'clean' and 'unknown',
>> respectively. However, the 'ignored', 'clean' and 'unknown' themselves
>> get a different meaning further down in the method, so use the list*
>> names for clarity, since they only have one meaning.
>
>
> AFAICS it is only 'clean' that is used for other purposes later on. (It was
> changed recently when unused values were given a _ prefix.) 'clean' starts
> out as a parameter in a monkey patched wrapper function so it should keep
> its name.

Agreed.

> I think the real problem is that we reuse 'clean' later on - it
> would be better to fix that and avoid introducing the 'list*' aliases.

The same pattern as in reposetup.py is actually used in context.py and
dirstate.py. In both of those methods, all the variables get
reassigned different meaning. I'll just drop this patch and maybe I'll
address it in a different way later.

Patch

diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py
+++ b/hgext/largefiles/reposetup.py
@@ -152,7 +152,7 @@ 
                     m._files = tostandins(m._files)
 
                     result = super(lfilesrepo, self).status(node1, node2, m,
-                        ignored, clean, unknown, listsubrepos)
+                        listignored, listclean, listunknown, listsubrepos)
                     if working:
 
                         def sfindirstate(f):