Patchwork [02,of,10,stable] largefiles: adopt verify to batched remote statlfile (issue3780)

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 25, 2013, 5:17 a.m.
Message ID <d956326990d42e04627d.1359091021@mk-desktop>
Download mbox | patch
Permalink /patch/722/
State Deferred, archived
Headers show

Comments

Mads Kiilerich - Jan. 25, 2013, 5:17 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1359090858 -3600
# Branch stable
# Node ID d956326990d42e04627d0d8077380c6f69921132
# Parent  4ac7648ee50c5a799fec5cd2c48d89b99b6dd044
largefiles: adopt verify to batched remote statlfile (issue3780)

9e1616307c4c introduced batching of statlfile, but not all codepaths got
converted.

'hg verify' with a remotestore could thus crash with
  TypeError: 'builtin_function_or_method' object is not iterable

Also, the 'hash' variable was used without assigning to it. Don't use variable
names that collide with Python built-in functions. Instead we use 'expecthash'
as in localstore.
Dave S - Jan. 25, 2013, 8:18 a.m.
On Thu, Jan 24, 2013 at 9:17 PM, Mads Kiilerich <mads@kiilerich.com> wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1359090858 -3600
> # Branch stable
> # Node ID d956326990d42e04627d0d8077380c6f69921132
> # Parent  4ac7648ee50c5a799fec5cd2c48d89b99b6dd044
> largefiles: adopt verify to batched remote statlfile (issue3780)
>
> 9e1616307c4c introduced batching of statlfile, but not all codepaths got
> converted.
>
> 'hg verify' with a remotestore could thus crash with
>   TypeError: 'builtin_function_or_method' object is not iterable

If I'm understanding this correctly, I think you mean "adapt" rather
than "adopt".  (For #03, also.)

Am I on the right track?

/dps
Mads Kiilerich - Jan. 25, 2013, 1:41 p.m.
On 01/25/2013 09:18 AM, Dave S wrote:
> On Thu, Jan 24, 2013 at 9:17 PM, Mads Kiilerich <mads@kiilerich.com> wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1359090858 -3600
>> # Branch stable
>> # Node ID d956326990d42e04627d0d8077380c6f69921132
>> # Parent  4ac7648ee50c5a799fec5cd2c48d89b99b6dd044
>> largefiles: adopt verify to batched remote statlfile (issue3780)
>>
>> 9e1616307c4c introduced batching of statlfile, but not all codepaths got
>> converted.
>>
>> 'hg verify' with a remotestore could thus crash with
>>    TypeError: 'builtin_function_or_method' object is not iterable
> If I'm understanding this correctly, I think you mean "adapt" rather
> than "adopt".  (For #03, also.)
>
> Am I on the right track?

yeah, thanks

/Mads
Pierre-Yves David - Jan. 28, 2013, 1:25 p.m.
On Fri, Jan 25, 2013 at 06:17:01AM +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1359090858 -3600
> # Branch stable
> # Node ID d956326990d42e04627d0d8077380c6f69921132
> # Parent  4ac7648ee50c5a799fec5cd2c48d89b99b6dd044
> largefiles: adopt verify to batched remote statlfile (issue3780)
> 
> 9e1616307c4c introduced batching of statlfile, but not all codepaths got
> converted.
> 
> 'hg verify' with a remotestore could thus crash with
>   TypeError: 'builtin_function_or_method' object is not iterable
> 
> Also, the 'hash' variable was used without assigning to it. Don't use variable
> names that collide with Python built-in functions. Instead we use 'expecthash'
> as in localstore.
> 
> diff --git a/hgext/largefiles/remotestore.py b/hgext/largefiles/remotestore.py
> --- a/hgext/largefiles/remotestore.py
> +++ b/hgext/largefiles/remotestore.py
> @@ -87,7 +87,8 @@
>  
>          verified.add(key)
>  
> -        stat = self._stat(hash)
> +        expecthash = fctx.data()[0:40]

n00b question: why are we accessing [0:40] here? aren't the data content in a
parsable format ?

> +        stat = self._stat([expecthash])[expecthash]
>          if not stat:
>              return False
>          elif stat == 1:
> diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
> --- a/tests/test-largefiles.t
> +++ b/tests/test-largefiles.t
> @@ -1539,10 +1539,38 @@
>    remote: adding manifests
>    remote: adding file changes
>    remote: added 1 changesets with 1 changes to 1 files
> +  $ [ -f "${USERCACHE}"/02a439e5c31c526465ab1a0ca1f431f76b827b90 ]
> +  $ [ -f empty/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90 ]
>  
> -Clone over http, with largefiles being pulled on update, not on clone.
> +Clone over http, no largefiles pulled on clone.
>  
> -  $ hg clone -q http://localhost:$HGPORT2/ http-clone -U
> +  $ hg clone http://localhost:$HGPORT2/ http-clone -U
> +  requesting all changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 1 changesets with 1 changes to 1 files
> +
> +verify remotestore verify:
> +
> +  $ mv "${USERCACHE}"/02a439e5c31c526465ab1a0ca1f431f76b827b90 .
> +  $ rm empty/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90
> +  $ hg -R http-clone verify --large --lfa
> +  checking changesets
> +  checking manifests
> +  crosschecking files in changesets and manifests
> +  checking files
> +  1 files, 1 changesets, 1 total revisions
> +  searching 1 changesets for largefiles
> +  changeset 0:cf03e5bb9936: f1 missing
> +  verified existence of 1 revisions of 1 largefiles
> +  [1]
> +  $ mv 02a439e5c31c526465ab1a0ca1f431f76b827b90 "${USERCACHE}"/
> +  $ hg -R http-clone -q verify --large --lfa
> +  searching 1 changesets for largefiles
> +  verified existence of 1 revisions of 1 largefiles
> +
> +largefiles pulled on update:
>  
>    $ hg -R http-clone --debug up --config largefiles.usercache=http-clone-usercache
>    resolving manifests
> @@ -1562,8 +1590,19 @@
>    1 largefiles updated, 0 removed
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>  
> -  $ ls http-clone-usercache/*
> -  http-clone-usercache/02a439e5c31c526465ab1a0ca1f431f76b827b90
> +undo update and its pull
> +  $ hg -R http-clone up -q -r null
> +  $ rm http-clone-usercache/02a439e5c31c526465ab1a0ca1f431f76b827b90
> +  $ rm http-clone/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90
> +make largefile missing on server
> +  $ rm empty/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90
> +  $ rm "${USERCACHE}"/02a439e5c31c526465ab1a0ca1f431f76b827b90
> +try again and expect error
> +  $ hg -R http-clone up
> +  getting changed largefiles
> +  error getting id 02a439e5c31c526465ab1a0ca1f431f76b827b90 from url http://localhost:$HGPORT2/ for file f1: HTTP Error 500: Internal Server Error

Why do we 500 here? a comment to clarify this would be helpful.
Mads Kiilerich - Jan. 28, 2013, 1:50 p.m.
On 01/27/2013 05:25 PM, Kevin Bullock wrote:
> On 24 Jan 2013, at 11:17 PM, Mads Kiilerich wrote:
>
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1359090858 -3600
>> # Branch stable
>> # Node ID d956326990d42e04627d0d8077380c6f69921132
>> # Parent  4ac7648ee50c5a799fec5cd2c48d89b99b6dd044
>> largefiles: adopt verify to batched remote statlfile (issue3780)
>>
>> 9e1616307c4c introduced batching of statlfile, but not all codepaths got
>> converted.
>>
>> 'hg verify' with a remotestore could thus crash with
>>   TypeError: 'builtin_function_or_method' object is not iterable
>>
>> Also, the 'hash' variable was used without assigning to it. Don't use variable
>> names that collide with Python built-in functions. Instead we use 'expecthash'
>> as in localstore.
>>
>> diff --git a/hgext/largefiles/remotestore.py b/hgext/largefiles/remotestore.py
>> --- a/hgext/largefiles/remotestore.py
>> +++ b/hgext/largefiles/remotestore.py
>> @@ -87,7 +87,8 @@
>>
>>          verified.add(key)
>>
>> -        stat = self._stat(hash)
>> +        expecthash = fctx.data()[0:40]
>> +        stat = self._stat([expecthash])[expecthash]
> Hmm, this call pattern starts to seem like a code smell.
>
> 1. Should we provide a _statfile(hash) to verify just one file like used to be possible?
>
> 2. Alternatively, are we defeating batching here on purpose? Should this be refactored to enable batching?

Yes, it smells. But it is no longer dead - it just smells funny.

For a stable fix I prefer to make a minimal change and duplicate 
existing patterns - this and a lot of other things should be cleaned up 
later.

/Mads
Mads Kiilerich - Jan. 28, 2013, 1:57 p.m.
On 01/28/2013 02:25 PM, Pierre-Yves David wrote:
> On Fri, Jan 25, 2013 at 06:17:01AM +0100, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1359090858 -3600
>> # Branch stable
>> # Node ID d956326990d42e04627d0d8077380c6f69921132
>> # Parent  4ac7648ee50c5a799fec5cd2c48d89b99b6dd044
>> largefiles: adopt verify to batched remote statlfile (issue3780)
>>
>> 9e1616307c4c introduced batching of statlfile, but not all codepaths got
>> converted.
>>
>> 'hg verify' with a remotestore could thus crash with
>>    TypeError: 'builtin_function_or_method' object is not iterable
>>
>> Also, the 'hash' variable was used without assigning to it. Don't use variable
>> names that collide with Python built-in functions. Instead we use 'expecthash'
>> as in localstore.
>>
>> diff --git a/hgext/largefiles/remotestore.py b/hgext/largefiles/remotestore.py
>> --- a/hgext/largefiles/remotestore.py
>> +++ b/hgext/largefiles/remotestore.py
>> @@ -87,7 +87,8 @@
>>   
>>           verified.add(key)
>>   
>> -        stat = self._stat(hash)
>> +        expecthash = fctx.data()[0:40]
> n00b question: why are we accessing [0:40] here? aren't the data content in a
> parsable format ?

I don't _really_ know. But that is how it is done elsewhere. I guess it 
is to ignore trailing newlines.

>> +try again and expect error
>> +  $ hg -R http-clone up
>> +  getting changed largefiles
>> +  error getting id 02a439e5c31c526465ab1a0ca1f431f76b827b90 from url http://localhost:$HGPORT2/ for file f1: HTTP Error 500: Internal Server Error
> Why do we 500 here? a comment to clarify this would be helpful.

Because _getfile doesn't stat correctly - it will stat the wrong files 
but happily misunderstand the result and try to download a file that 
doesn't exist.

Fixed in "largefiles: adapt remotestore._getfile to batched statlfile".


In a perfect world the server would respond in a more sensible way ...


/Mads
Pierre-Yves David - Jan. 28, 2013, 2:04 p.m.
On Mon, Jan 28, 2013 at 02:57:03PM +0100, Mads Kiilerich wrote:
> On 01/28/2013 02:25 PM, Pierre-Yves David wrote:
> >On Fri, Jan 25, 2013 at 06:17:01AM +0100, Mads Kiilerich wrote:
> >># HG changeset patch
> >># User Mads Kiilerich <madski@unity3d.com>
> >># Date 1359090858 -3600
> >># Branch stable
> >># Node ID d956326990d42e04627d0d8077380c6f69921132
> >># Parent  4ac7648ee50c5a799fec5cd2c48d89b99b6dd044
> >>largefiles: adopt verify to batched remote statlfile (issue3780)
> >>
> >>9e1616307c4c introduced batching of statlfile, but not all codepaths got
> >>converted.
> >>
> >>'hg verify' with a remotestore could thus crash with
> >>   TypeError: 'builtin_function_or_method' object is not iterable
> >>
> >>Also, the 'hash' variable was used without assigning to it. Don't use variable
> >>names that collide with Python built-in functions. Instead we use 'expecthash'
> >>as in localstore.
> >>
> >>diff --git a/hgext/largefiles/remotestore.py b/hgext/largefiles/remotestore.py
> >>--- a/hgext/largefiles/remotestore.py
> >>+++ b/hgext/largefiles/remotestore.py
> >>@@ -87,7 +87,8 @@
> >>          verified.add(key)
> >>-        stat = self._stat(hash)
> >>+        expecthash = fctx.data()[0:40]
> >n00b question: why are we accessing [0:40] here? aren't the data content in a
> >parsable format ?
> 
> I don't _really_ know. But that is how it is done elsewhere. I guess
> it is to ignore trailing newlines.

Ok, lets says people had good reason to do it this way, looks elsewhere and carry on. This series in an overall.

> >>+try again and expect error
> >>+  $ hg -R http-clone up
> >>+  getting changed largefiles
> >>+  error getting id 02a439e5c31c526465ab1a0ca1f431f76b827b90 from url http://localhost:$HGPORT2/ for file f1: HTTP Error 500: Internal Server Error
> >Why do we 500 here? a comment to clarify this would be helpful.
> 
> Because _getfile doesn't stat correctly - it will stat the wrong
> files but happily misunderstand the result and try to download a
> file that doesn't exist.
> 
> Fixed in "largefiles: adapt remotestore._getfile to batched statlfile".
> 
> 
> In a perfect world the server would respond in a more sensible way ...

I would expand the comment to explain that the error is not actually wished.

Patch

diff --git a/hgext/largefiles/remotestore.py b/hgext/largefiles/remotestore.py
--- a/hgext/largefiles/remotestore.py
+++ b/hgext/largefiles/remotestore.py
@@ -87,7 +87,8 @@ 
 
         verified.add(key)
 
-        stat = self._stat(hash)
+        expecthash = fctx.data()[0:40]
+        stat = self._stat([expecthash])[expecthash]
         if not stat:
             return False
         elif stat == 1:
diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
--- a/tests/test-largefiles.t
+++ b/tests/test-largefiles.t
@@ -1539,10 +1539,38 @@ 
   remote: adding manifests
   remote: adding file changes
   remote: added 1 changesets with 1 changes to 1 files
+  $ [ -f "${USERCACHE}"/02a439e5c31c526465ab1a0ca1f431f76b827b90 ]
+  $ [ -f empty/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90 ]
 
-Clone over http, with largefiles being pulled on update, not on clone.
+Clone over http, no largefiles pulled on clone.
 
-  $ hg clone -q http://localhost:$HGPORT2/ http-clone -U
+  $ hg clone http://localhost:$HGPORT2/ http-clone -U
+  requesting all changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+
+verify remotestore verify:
+
+  $ mv "${USERCACHE}"/02a439e5c31c526465ab1a0ca1f431f76b827b90 .
+  $ rm empty/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90
+  $ hg -R http-clone verify --large --lfa
+  checking changesets
+  checking manifests
+  crosschecking files in changesets and manifests
+  checking files
+  1 files, 1 changesets, 1 total revisions
+  searching 1 changesets for largefiles
+  changeset 0:cf03e5bb9936: f1 missing
+  verified existence of 1 revisions of 1 largefiles
+  [1]
+  $ mv 02a439e5c31c526465ab1a0ca1f431f76b827b90 "${USERCACHE}"/
+  $ hg -R http-clone -q verify --large --lfa
+  searching 1 changesets for largefiles
+  verified existence of 1 revisions of 1 largefiles
+
+largefiles pulled on update:
 
   $ hg -R http-clone --debug up --config largefiles.usercache=http-clone-usercache
   resolving manifests
@@ -1562,8 +1590,19 @@ 
   1 largefiles updated, 0 removed
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
-  $ ls http-clone-usercache/*
-  http-clone-usercache/02a439e5c31c526465ab1a0ca1f431f76b827b90
+undo update and its pull
+  $ hg -R http-clone up -q -r null
+  $ rm http-clone-usercache/02a439e5c31c526465ab1a0ca1f431f76b827b90
+  $ rm http-clone/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90
+make largefile missing on server
+  $ rm empty/.hg/largefiles/02a439e5c31c526465ab1a0ca1f431f76b827b90
+  $ rm "${USERCACHE}"/02a439e5c31c526465ab1a0ca1f431f76b827b90
+try again and expect error
+  $ hg -R http-clone up
+  getting changed largefiles
+  error getting id 02a439e5c31c526465ab1a0ca1f431f76b827b90 from url http://localhost:$HGPORT2/ for file f1: HTTP Error 500: Internal Server Error
+  0 largefiles updated, 0 removed
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
   $ rm -rf empty http-clone http-clone-usercache