| 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
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.
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
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.
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: