Patchwork [remotefilelog-ext] fileserverclient: avoid ever requesting nullid nodes

login
register
mail settings
Submitter Augie Fackler
Date Oct. 24, 2016, 10:44 p.m.
Message ID <c282b94ef20c036ac6d6.1477349071@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/17186/
State Superseded
Headers show

Comments

Augie Fackler - Oct. 24, 2016, 10:44 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1477347305 14400
#      Mon Oct 24 18:15:05 2016 -0400
# Node ID c282b94ef20c036ac6d68eca5ce07077eb61fcb9
# Parent  be571104d9f7292cd218dbb1aaebda9ac0b6315f
fileserverclient: avoid ever requesting nullid nodes

I keep tripping over bugs where this angers some part of our
stack. It's dumb to even be fetching these, but it's harmless to skip
them, so issue a developer warning when we encounter one and refuse to
fetch it.
Jun Wu - Oct. 24, 2016, 11:06 p.m.
Other than "len(nullids)", the patch looks good to me. I think we can fix it
in flight.

Excerpts from Augie Fackler's message of 2016-10-24 18:44:31 -0400:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1477347305 14400
> #      Mon Oct 24 18:15:05 2016 -0400
> # Node ID c282b94ef20c036ac6d68eca5ce07077eb61fcb9
> # Parent  be571104d9f7292cd218dbb1aaebda9ac0b6315f
> fileserverclient: avoid ever requesting nullid nodes
> 
> I keep tripping over bugs where this angers some part of our
> stack. It's dumb to even be fetching these, but it's harmless to skip
> them, so issue a developer warning when we encounter one and refuse to
> fetch it.
> 
> diff --git a/remotefilelog/fileserverclient.py b/remotefilelog/fileserverclient.py
> --- a/remotefilelog/fileserverclient.py
> +++ b/remotefilelog/fileserverclient.py
> @@ -577,6 +577,15 @@ class fileserverclient(object):
>          if fetchhistory:
>              missingids.update(historystore.getmissing(idstocheck))
>  
> +        # partition missing nodes into nullid and not-nullid so we can
> +        # warn about this filtering potentially shadowing bugs.
> +        nullids = len([None for unused, id in missingids if id == node.nullid])
> +        if nullids:
> +            missingids = [(f, id) for f, id in missingids if id != node.nullid]
> +            repo.ui.develwarn(
> +                ('remotefilelog not fetching %d null revs'
> +                 ' - this is likely hiding bugs' % len(nullids)),
                                                      ~~~~~~~~~~~~
                                                 probably just "nullids"
> +                config='remotefilelog-ext')
>          if missingids:
>              global fetches, fetched, fetchcost
>              fetches += 1
Augie Fackler - Oct. 25, 2016, 12:35 a.m.
> On Oct 24, 2016, at 19:06, Jun Wu <quark@fb.com> wrote:
> 
> Other than "len(nullids)", the patch looks good to me. I think we can fix it
> in flight.
> 
> Excerpts from Augie Fackler's message of 2016-10-24 18:44:31 -0400:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1477347305 14400
>> #      Mon Oct 24 18:15:05 2016 -0400
>> # Node ID c282b94ef20c036ac6d68eca5ce07077eb61fcb9
>> # Parent  be571104d9f7292cd218dbb1aaebda9ac0b6315f
>> fileserverclient: avoid ever requesting nullid nodes
>> 
>> I keep tripping over bugs where this angers some part of our
>> stack. It's dumb to even be fetching these, but it's harmless to skip
>> them, so issue a developer warning when we encounter one and refuse to
>> fetch it.
>> 
>> diff --git a/remotefilelog/fileserverclient.py b/remotefilelog/fileserverclient.py
>> --- a/remotefilelog/fileserverclient.py
>> +++ b/remotefilelog/fileserverclient.py
>> @@ -577,6 +577,15 @@ class fileserverclient(object):
>>         if fetchhistory:
>>             missingids.update(historystore.getmissing(idstocheck))
>> 
>> +        # partition missing nodes into nullid and not-nullid so we can
>> +        # warn about this filtering potentially shadowing bugs.
>> +        nullids = len([None for unused, id in missingids if id == node.nullid])
>> +        if nullids:
>> +            missingids = [(f, id) for f, id in missingids if id != node.nullid]
>> +            repo.ui.develwarn(
>> +                ('remotefilelog not fetching %d null revs'
>> +                 ' - this is likely hiding bugs' % len(nullids)),
>                                                      ~~~~~~~~~~~~
>                                                 probably just "null ids"

Ugh, yep, good catch. I'll mail a v2 for ease of application.

>> +                config='remotefilelog-ext')
>>         if missingids:
>>             global fetches, fetched, fetchcost
>>             fetches += 1

Patch

diff --git a/remotefilelog/fileserverclient.py b/remotefilelog/fileserverclient.py
--- a/remotefilelog/fileserverclient.py
+++ b/remotefilelog/fileserverclient.py
@@ -577,6 +577,15 @@  class fileserverclient(object):
         if fetchhistory:
             missingids.update(historystore.getmissing(idstocheck))
 
+        # partition missing nodes into nullid and not-nullid so we can
+        # warn about this filtering potentially shadowing bugs.
+        nullids = len([None for unused, id in missingids if id == node.nullid])
+        if nullids:
+            missingids = [(f, id) for f, id in missingids if id != node.nullid]
+            repo.ui.develwarn(
+                ('remotefilelog not fetching %d null revs'
+                 ' - this is likely hiding bugs' % len(nullids)),
+                config='remotefilelog-ext')
         if missingids:
             global fetches, fetched, fetchcost
             fetches += 1