Patchwork [8,of,8] largefiles: inline redundant toname function in status

login
register
mail settings
Submitter Mads Kiilerich
Date Oct. 15, 2014, 3:09 a.m.
Message ID <defd3115ade0f094bcbd.1413342555@localhost.localdomain>
Download mbox | patch
Permalink /patch/6273/
State Accepted
Headers show

Comments

Mads Kiilerich - Oct. 15, 2014, 3:09 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1413342536 -7200
#      Wed Oct 15 05:08:56 2014 +0200
# Node ID defd3115ade0f094bcbd2f380cfffa3df5705b04
# Parent  05f6422005091798fbe0f2a8d4d5d66ca2c78f28
largefiles: inline redundant toname function in status

Simpler and an optimization.
Martin von Zweigbergk - Oct. 15, 2014, 4:50 a.m.
On Tue, Oct 14, 2014 at 8:09 PM, Mads Kiilerich <mads@kiilerich.com> wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1413342536 -7200
> #      Wed Oct 15 05:08:56 2014 +0200
> # Node ID defd3115ade0f094bcbd2f380cfffa3df5705b04
> # Parent  05f6422005091798fbe0f2a8d4d5d66ca2c78f28
> largefiles: inline redundant toname function in status
>
> Simpler and an optimization.
>
> diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
> --- a/hgext/largefiles/reposetup.py
> +++ b/hgext/largefiles/reposetup.py
> @@ -217,12 +217,8 @@ def reposetup(ui, repo):
>                                  clean)
>                      result = [sorted(list1 + list2)
>                                for (list1, list2) in zip(normals, lfstatus)]
> -                else:
> -                    def toname(f):
> -                        if lfutil.isstandin(f):
> -                            return lfutil.splitstandin(f)
> -                        return f
> -                    result = [[toname(f) for f in items]
> +                else: # not against working directory
> +                    result = [[lfutil.splitstandin(f) or f for f in items]

It's not obvious to me why this is equivalent. Are all the files in
'items' standins (so lfutil.isstandin() returns True)?

FWIW, 1-7 look good to me, although I don't know if number 2 (the mq
patch) was much of an improvement; I thought it looked fine both
before and after.
Mads Kiilerich - Oct. 15, 2014, 10:55 a.m.
On 10/15/2014 06:50 AM, Martin von Zweigbergk wrote:
> On Tue, Oct 14, 2014 at 8:09 PM, Mads Kiilerich <mads@kiilerich.com> wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1413342536 -7200
>> #      Wed Oct 15 05:08:56 2014 +0200
>> # Node ID defd3115ade0f094bcbd2f380cfffa3df5705b04
>> # Parent  05f6422005091798fbe0f2a8d4d5d66ca2c78f28
>> largefiles: inline redundant toname function in status
>>
>> Simpler and an optimization.
>>
>> diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
>> --- a/hgext/largefiles/reposetup.py
>> +++ b/hgext/largefiles/reposetup.py
>> @@ -217,12 +217,8 @@ def reposetup(ui, repo):
>>                                   clean)
>>                       result = [sorted(list1 + list2)
>>                                 for (list1, list2) in zip(normals, lfstatus)]
>> -                else:
>> -                    def toname(f):
>> -                        if lfutil.isstandin(f):
>> -                            return lfutil.splitstandin(f)
>> -                        return f
>> -                    result = [[toname(f) for f in items]
>> +                else: # not against working directory
>> +                    result = [[lfutil.splitstandin(f) or f for f in items]
> It's not obvious to me why this is equivalent. Are all the files in
> 'items' standins (so lfutil.isstandin() returns True)?

splitstandin will return None if there is no standin to split - and then 
the boolean short-circuit evaluation kicks in and turns it into f.

Doing it this way avoids a couple of function calls and double stripping 
of leading .hglf . This is in a tight loop and is worth optimizing.

I also find this elegant and more readable than reading through toname 
and figure out what it is doing ... but opinions might vary on that.

> FWIW, 1-7 look good to me, although I don't know if number 2 (the mq
> patch) was much of an improvement; I thought it looked fine both
> before and after.

It might be. "directory...done" looked so unintentional that I didn't 
consider that option. Usually all our messages ends with a newline - it 
is better to stick to that convention and avoid problems with 
interleaving of different messages. In this case I had added debug 
statements to code that ended up being executed before the "done" - that 
gave weird output.

/Mads
Martin von Zweigbergk - Oct. 15, 2014, 3:06 p.m.
On Wed, Oct 15, 2014 at 3:55 AM, Mads Kiilerich <mads@kiilerich.com> wrote:
> On 10/15/2014 06:50 AM, Martin von Zweigbergk wrote:
>>
>> On Tue, Oct 14, 2014 at 8:09 PM, Mads Kiilerich <mads@kiilerich.com>
>> wrote:
>>>
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1413342536 -7200
>>> #      Wed Oct 15 05:08:56 2014 +0200
>>> # Node ID defd3115ade0f094bcbd2f380cfffa3df5705b04
>>> # Parent  05f6422005091798fbe0f2a8d4d5d66ca2c78f28
>>> largefiles: inline redundant toname function in status
>>>
>>> Simpler and an optimization.
>>>
>>> diff --git a/hgext/largefiles/reposetup.py
>>> b/hgext/largefiles/reposetup.py
>>> --- a/hgext/largefiles/reposetup.py
>>> +++ b/hgext/largefiles/reposetup.py
>>> @@ -217,12 +217,8 @@ def reposetup(ui, repo):
>>>                                   clean)
>>>                       result = [sorted(list1 + list2)
>>>                                 for (list1, list2) in zip(normals,
>>> lfstatus)]
>>> -                else:
>>> -                    def toname(f):
>>> -                        if lfutil.isstandin(f):
>>> -                            return lfutil.splitstandin(f)
>>> -                        return f
>>> -                    result = [[toname(f) for f in items]
>>> +                else: # not against working directory
>>> +                    result = [[lfutil.splitstandin(f) or f for f in
>>> items]
>>
>> It's not obvious to me why this is equivalent. Are all the files in
>> 'items' standins (so lfutil.isstandin() returns True)?
>
>
> splitstandin will return None if there is no standin to split - and then the
> boolean short-circuit evaluation kicks in and turns it into f.

Oh, that explains it. I had missed the "or f" despite looking at it
multiple times. Looks good then.
Matt Mackall - Oct. 18, 2014, 8:25 p.m.
On Wed, 2014-10-15 at 05:09 +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1413342536 -7200
> #      Wed Oct 15 05:08:56 2014 +0200
> # Node ID defd3115ade0f094bcbd2f380cfffa3df5705b04
> # Parent  05f6422005091798fbe0f2a8d4d5d66ca2c78f28
> largefiles: inline redundant toname function in status

3-8 queued, thanks.

Patch

diff --git a/hgext/largefiles/reposetup.py b/hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py
+++ b/hgext/largefiles/reposetup.py
@@ -217,12 +217,8 @@  def reposetup(ui, repo):
                                 clean)
                     result = [sorted(list1 + list2)
                               for (list1, list2) in zip(normals, lfstatus)]
-                else:
-                    def toname(f):
-                        if lfutil.isstandin(f):
-                            return lfutil.splitstandin(f)
-                        return f
-                    result = [[toname(f) for f in items]
+                else: # not against working directory
+                    result = [[lfutil.splitstandin(f) or f for f in items]
                               for items in result]
 
                 if wlock: