Patchwork [2,of,2] repair: combine two loops over changelog revisions

login
register
mail settings
Submitter via Mercurial-devel
Date Jan. 4, 2017, 6:52 p.m.
Message ID <c9c5864ddd54b240692a.1483555978@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/18095/
State Accepted
Headers show

Comments

via Mercurial-devel - Jan. 4, 2017, 6:52 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1483554904 28800
#      Wed Jan 04 10:35:04 2017 -0800
# Node ID c9c5864ddd54b240692a2712d59227c9d4ca2f99
# Parent  acf0037a9a5bb14603bb50c57822d4ab5896e5b5
repair: combine two loops over changelog revisions

This just saves a few lines.
Pierre-Yves David - Jan. 4, 2017, 7:06 p.m.
On 01/04/2017 07:52 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1483554904 28800
> #      Wed Jan 04 10:35:04 2017 -0800
> # Node ID c9c5864ddd54b240692a2712d59227c9d4ca2f99
> # Parent  acf0037a9a5bb14603bb50c57822d4ab5896e5b5
> repair: combine two loops over changelog revisions
>
> This just saves a few lines.

Beware that cl.revs(start) might be different from
xrange(start, len(cl)) if filtering come into play.

I would vaguely assume that strip is enforcing it run on an unfiltered 
changelog, but can you double check that? (In particular, it is 
suspicious that we have a difference here why would we have move of the 
call to cl.revs(…) and not the other one ?)
  If not already the case, It is probably worth dropping an 
assert+comment (or a ProgrammingError) to protect that assertion.

Cheers,

> diff -r acf0037a9a5b -r c9c5864ddd54 mercurial/repair.py
> --- a/mercurial/repair.py	Wed Jan 04 10:07:12 2017 -0800
> +++ b/mercurial/repair.py	Wed Jan 04 10:35:04 2017 -0800
> @@ -91,6 +91,9 @@
>      striplist = [cl.rev(node) for node in nodelist]
>      striprev = min(striplist)
>
> +    files = _collectfiles(repo, striprev)
> +    saverevs = _collectbrokencsets(repo, files, striprev)
> +
>      # Some revisions with rev > striprev may not be descendants of striprev.
>      # We have to find these revisions and put them in a bundle, so that
>      # we can restore them after the truncations.
> @@ -99,16 +102,11 @@
>      # (head = revision in the set that has no descendant in the set;
>      #  base = revision in the set that has no ancestor in the set)
>      tostrip = set(striplist)
> +    saveheads = set(saverevs)
>      for r in cl.revs(start=striprev + 1):
>          if any(p in tostrip for p in cl.parentrevs(r)):
>              tostrip.add(r)
>
> -    files = _collectfiles(repo, striprev)
> -    saverevs = _collectbrokencsets(repo, files, striprev)
> -
> -    # compute heads
> -    saveheads = set(saverevs)
> -    for r in xrange(striprev + 1, len(cl)):
>          if r not in tostrip:
>              saverevs.add(r)
>              saveheads.difference_update(cl.parentrevs(r))
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - Jan. 4, 2017, 7:09 p.m.
On Wed, Jan 4, 2017 at 11:06 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 01/04/2017 07:52 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1483554904 28800
>> #      Wed Jan 04 10:35:04 2017 -0800
>> # Node ID c9c5864ddd54b240692a2712d59227c9d4ca2f99
>> # Parent  acf0037a9a5bb14603bb50c57822d4ab5896e5b5
>> repair: combine two loops over changelog revisions
>>
>> This just saves a few lines.
>
>
> Beware that cl.revs(start) might be different from
> xrange(start, len(cl)) if filtering come into play.

Good point.

>
> I would vaguely assume that strip is enforcing it run on an unfiltered
> changelog, but can you double check that?

Yep, about 20 lines up, you'll find these two lines:

    repo = repo.unfiltered()
[...]
    cl = repo.changelog



> (In particular, it is suspicious
> that we have a difference here why would we have move of the call to
> cl.revs(…) and not the other one ?)
>  If not already the case, It is probably worth dropping an assert+comment
> (or a ProgrammingError) to protect that assertion.
>
> Cheers,
>
>
>> diff -r acf0037a9a5b -r c9c5864ddd54 mercurial/repair.py
>> --- a/mercurial/repair.py       Wed Jan 04 10:07:12 2017 -0800
>> +++ b/mercurial/repair.py       Wed Jan 04 10:35:04 2017 -0800
>> @@ -91,6 +91,9 @@
>>      striplist = [cl.rev(node) for node in nodelist]
>>      striprev = min(striplist)
>>
>> +    files = _collectfiles(repo, striprev)
>> +    saverevs = _collectbrokencsets(repo, files, striprev)
>> +
>>      # Some revisions with rev > striprev may not be descendants of
>> striprev.
>>      # We have to find these revisions and put them in a bundle, so that
>>      # we can restore them after the truncations.
>> @@ -99,16 +102,11 @@
>>      # (head = revision in the set that has no descendant in the set;
>>      #  base = revision in the set that has no ancestor in the set)
>>      tostrip = set(striplist)
>> +    saveheads = set(saverevs)
>>      for r in cl.revs(start=striprev + 1):
>>          if any(p in tostrip for p in cl.parentrevs(r)):
>>              tostrip.add(r)
>>
>> -    files = _collectfiles(repo, striprev)
>> -    saverevs = _collectbrokencsets(repo, files, striprev)
>> -
>> -    # compute heads
>> -    saveheads = set(saverevs)
>> -    for r in xrange(striprev + 1, len(cl)):
>>          if r not in tostrip:
>>              saverevs.add(r)
>>              saveheads.difference_update(cl.parentrevs(r))
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
> --
> Pierre-Yves David
Pierre-Yves David - Jan. 6, 2017, 6:25 a.m.
On 01/04/2017 08:09 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> On Wed, Jan 4, 2017 at 11:06 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 01/04/2017 07:52 PM, Martin von Zweigbergk via Mercurial-devel wrote:
>>>
>>> # HG changeset patch
>>> # User Martin von Zweigbergk <martinvonz@google.com>
>>> # Date 1483554904 28800
>>> #      Wed Jan 04 10:35:04 2017 -0800
>>> # Node ID c9c5864ddd54b240692a2712d59227c9d4ca2f99
>>> # Parent  acf0037a9a5bb14603bb50c57822d4ab5896e5b5
>>> repair: combine two loops over changelog revisions
>>>
>>> This just saves a few lines.
>>
>>
>> Beware that cl.revs(start) might be different from
>> xrange(start, len(cl)) if filtering come into play.
>
> Good point.
>
>>
>> I would vaguely assume that strip is enforcing it run on an unfiltered
>> changelog, but can you double check that?
>
> Yep, about 20 lines up, you'll find these two lines:
>
>     repo = repo.unfiltered()
> [...]
>     cl = repo.changelog

Thanks for double checking, these are pushed.

Cheers,

Patch

diff -r acf0037a9a5b -r c9c5864ddd54 mercurial/repair.py
--- a/mercurial/repair.py	Wed Jan 04 10:07:12 2017 -0800
+++ b/mercurial/repair.py	Wed Jan 04 10:35:04 2017 -0800
@@ -91,6 +91,9 @@ 
     striplist = [cl.rev(node) for node in nodelist]
     striprev = min(striplist)
 
+    files = _collectfiles(repo, striprev)
+    saverevs = _collectbrokencsets(repo, files, striprev)
+
     # Some revisions with rev > striprev may not be descendants of striprev.
     # We have to find these revisions and put them in a bundle, so that
     # we can restore them after the truncations.
@@ -99,16 +102,11 @@ 
     # (head = revision in the set that has no descendant in the set;
     #  base = revision in the set that has no ancestor in the set)
     tostrip = set(striplist)
+    saveheads = set(saverevs)
     for r in cl.revs(start=striprev + 1):
         if any(p in tostrip for p in cl.parentrevs(r)):
             tostrip.add(r)
 
-    files = _collectfiles(repo, striprev)
-    saverevs = _collectbrokencsets(repo, files, striprev)
-
-    # compute heads
-    saveheads = set(saverevs)
-    for r in xrange(striprev + 1, len(cl)):
         if r not in tostrip:
             saverevs.add(r)
             saveheads.difference_update(cl.parentrevs(r))