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

[Martin von Zweigbergk]

On Sat, Sep 20, 2014 at 9:32 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 09/17/2014 10:40 PM, Martin von Zweigbergk wrote:
>>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz at 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 at selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>
>