Patchwork [6,of,8,v2] largefiles: only reset status fields where needed

login
register
mail settings
Submitter Martin von Zweigbergk
Date Sept. 23, 2014, 9:46 p.m.
Message ID <06769b7fbba93d10fc2e.1411508771@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/5928/
State Changes Requested
Headers show

Comments

Martin von Zweigbergk - Sept. 23, 2014, 9:46 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@gmail.com>
# Date 1410906484 25200
#      Tue Sep 16 15:28:04 2014 -0700
# Node ID 06769b7fbba93d10fc2e7cf7967779eb838f61a6
# Parent  a6e0710be72f0df3e84ef410b9f34d86a9990bd4
largefiles: only reset status fields where needed

Part of lfilesrepo.status() looks as follows. If 'listunknown' (etc.)
is false, the result from orig() should naturally not contain any
unknown files. There is therefore no need to clear these fields unless
the <condition> path was taken, so move the clearing into that block.

  result = orig(node1, node2, m, listignored, listclean,
                listunknown, listsubrepos)
  if <condition>:
      // modify 'result'
  if not listunknown:
      result[4] = []
  // similar for ignored and clean files
  return result;
Mads Kiilerich - Sept. 23, 2014, 11:09 p.m.
On 09/23/2014 11:46 PM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@gmail.com>
> # Date 1410906484 25200
> #      Tue Sep 16 15:28:04 2014 -0700
> # Node ID 06769b7fbba93d10fc2e7cf7967779eb838f61a6
> # Parent  a6e0710be72f0df3e84ef410b9f34d86a9990bd4
> largefiles: only reset status fields where needed
>
> Part of lfilesrepo.status() looks as follows. If 'listunknown' (etc.)
> is false, the result from orig() should naturally not contain any
> unknown files. There is therefore no need to clear these fields unless
> the <condition> path was taken, so move the clearing into that block.
>
>    result = orig(node1, node2, m, listignored, listclean,
>                  listunknown, listsubrepos)
>    if <condition>:
>        // modify 'result'
>    if not listunknown:
>        result[4] = []
>    // similar for ignored and clean files
>    return result;
>
> diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
> --- a/hgext/largefiles/reposetup.py
> +++ b/hgext/largefiles/reposetup.py
> @@ -226,6 +226,12 @@
>                       lfiles = (modified, added, removed, missing, [], [], clean)
>                       result = [sorted(list1 + list2)
>                                 for (list1, list2) in zip(normals, lfiles)]
> +                    if not listunknown:
> +                        result[4] = []
> +                    if not listignored:
> +                        result[5] = []

It seemed to me like you agreed that at least these two assignments were 
redundant and misleading, but it seems like v2 of this patch is exactly 
like v1?

All the other patches in v2 looks good to me.

/Mads
Martin von Zweigbergk - Sept. 24, 2014, 4:15 p.m.
On Tue, Sep 23, 2014 at 4:09 PM, Mads Kiilerich <mads@kiilerich.com> wrote:
> On 09/23/2014 11:46 PM, Martin von Zweigbergk wrote:
>>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@gmail.com>
>> # Date 1410906484 25200
>> #      Tue Sep 16 15:28:04 2014 -0700
>> # Node ID 06769b7fbba93d10fc2e7cf7967779eb838f61a6
>> # Parent  a6e0710be72f0df3e84ef410b9f34d86a9990bd4
>> largefiles: only reset status fields where needed
>>
>> Part of lfilesrepo.status() looks as follows. If 'listunknown' (etc.)
>> is false, the result from orig() should naturally not contain any
>> unknown files. There is therefore no need to clear these fields unless
>> the <condition> path was taken, so move the clearing into that block.
>>
>>    result = orig(node1, node2, m, listignored, listclean,
>>                  listunknown, listsubrepos)
>>    if <condition>:
>>        // modify 'result'
>>    if not listunknown:
>>        result[4] = []
>>    // similar for ignored and clean files
>>    return result;
>>
>> diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
>> --- a/hgext/largefiles/reposetup.py
>> +++ b/hgext/largefiles/reposetup.py
>> @@ -226,6 +226,12 @@
>>                       lfiles = (modified, added, removed, missing, [], [],
>> clean)
>>                       result = [sorted(list1 + list2)
>>                                 for (list1, list2) in zip(normals,
>> lfiles)]
>> +                    if not listunknown:
>> +                        result[4] = []
>> +                    if not listignored:
>> +                        result[5] = []
>
>
> It seemed to me like you agreed that at least these two assignments were
> redundant and misleading, but it seems like v2 of this patch is exactly like
> v1?

Yes, I agree. My patch didn't make it more or less redundant or
misleading, so I planned on leaving the decision between clearing and
asserting for later. But I see how this patch is pointless if the next
patch is just going to remove the clearing (and I do prefer that over
asserting), so I'll send a v3 replacing this patch with one that
removes the clearing instead.

Patch

diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py
+++ b/hgext/largefiles/reposetup.py
@@ -226,6 +226,12 @@ 
                     lfiles = (modified, added, removed, missing, [], [], clean)
                     result = [sorted(list1 + list2)
                               for (list1, list2) in zip(normals, lfiles)]
+                    if not listunknown:
+                        result[4] = []
+                    if not listignored:
+                        result[5] = []
+                    if not listclean:
+                        result[6] = []
                 else:
                     def toname(f):
                         if lfutil.isstandin(f):
@@ -241,12 +247,6 @@ 
                 if wlock:
                     wlock.release()
 
-            if not listunknown:
-                result[4] = []
-            if not listignored:
-                result[5] = []
-            if not listclean:
-                result[6] = []
             self.lfstatus = True
             return result