Patchwork [7,of,9] largefiles: only reset status fields where needed

login
register
mail settings
Submitter Martin von Zweigbergk
Date Sept. 17, 2014, 8:40 p.m.
Message ID <013f8f20f2ae939ce4dd.1410986424@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/5860/
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 1410906484 25200
#      Tue Sep 16 15:28:04 2014 -0700
# Node ID 013f8f20f2ae939ce4dd773e2c4120182450459b
# Parent  20ce7e4eb36f84fce4b36b3bc838e9d170f77b42
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. 20, 2014, 4:32 p.m.
On 09/17/2014 10:40 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 013f8f20f2ae939ce4dd773e2c4120182450459b
> # Parent  20ce7e4eb36f84fce4b36b3bc838e9d170f77b42
> 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] = []

This is not wrong but it should also always be a noop. I think it would 
be better to assert that the lists are empty in these cases. Others 
might prefer to ride without safety belt and just let the invariant be 
implicit.

> +                    if not listclean:
> +                        result[6] = []

Right, this one is currently needed as we have a minor deficiency so we 
build the 'clean' list even without listclean. We should fix that and 
treat this like the two above.

Besides the comment to this and to patch 1, the series LGTM.

/Mads


>                   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
>   
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Sept. 22, 2014, 6:43 p.m.
On Sat, Sep 20, 2014 at 9:32 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 1410906484 25200
>> #      Tue Sep 16 15:28:04 2014 -0700
>> # Node ID 013f8f20f2ae939ce4dd773e2c4120182450459b
>> # Parent  20ce7e4eb36f84fce4b36b3bc838e9d170f77b42
>> 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] = []
>
>
> This is not wrong but it should also always be a noop. I think it would be
> better to assert that the lists are empty in these cases. Others might
> prefer to ride without safety belt and just let the invariant be implicit.

Right, it was a no-op before and after this patch. Also, what do you
mean by "assert"? "raise util.Abort"?

>
>> +                    if not listclean:
>> +                        result[6] = []
>
>
> Right, this one is currently needed as we have a minor deficiency so we
> build the 'clean' list even without listclean. We should fix that and treat
> this like the two above.

Maybe changing that and asserting in all three cases would be a patch on top?

> Besides the comment to this and to patch 1, the series LGTM.

Thanks for reviewing!

> /Mads
>
>
>>                   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
>>   _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>
>
Martin von Zweigbergk - Sept. 22, 2014, 6:45 p.m.
On Mon, Sep 22, 2014 at 11:43 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> On Sat, Sep 20, 2014 at 9:32 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 1410906484 25200
>>> #      Tue Sep 16 15:28:04 2014 -0700
>>> # Node ID 013f8f20f2ae939ce4dd773e2c4120182450459b
>>> # Parent  20ce7e4eb36f84fce4b36b3bc838e9d170f77b42
>>> 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] = []
>>
>>
>> This is not wrong but it should also always be a noop. I think it would be
>> better to assert that the lists are empty in these cases. Others might
>> prefer to ride without safety belt and just let the invariant be implicit.
>
> Right, it was a no-op before and after this patch.

> Also, what do you
> mean by "assert"? "raise util.Abort"?

Never mind! I should have just grepped for "assert" before I sent the
email. I assume you mean python's assert.

>
>>
>>> +                    if not listclean:
>>> +                        result[6] = []
>>
>>
>> Right, this one is currently needed as we have a minor deficiency so we
>> build the 'clean' list even without listclean. We should fix that and treat
>> this like the two above.
>
> Maybe changing that and asserting in all three cases would be a patch on top?
>
>> Besides the comment to this and to patch 1, the series LGTM.
>
> Thanks for reviewing!
>
>> /Mads
>>
>>
>>>                   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
>>>   _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel@selenic.com
>>> http://selenic.com/mailman/listinfo/mercurial-devel
>>
>>

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