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