Patchwork largefiles: check file in the repo store before checking remotely (issue4686)

# HG changeset patch
# User liscju <piotr.listkiewicz@gmail.com>
# Date 1466714237 -7200
#      Thu Jun 23 22:37:17 2016 +0200
# Node ID 2670e4331e0c763b04c0de2e1149917ccca42364
# Parent  6d96658a22b0fc1eb210c99c5629dd69fedf3006
largefiles: check file in the repo store before checking remotely (issue4686)

Problem was files to check were gathered in the repository where
the verify was launched but verification was done on the remote
store. It was observed when user committed in cloned repository
and ran verify before pushing - committed files were marked
as non existing.

This commit fixes this by checking in the remote store only files
that are not existing in the repository store where verify was launched.

Solution is similiar to fd288d118074

In commit message i wrote bad issue number, it should be 5257(verify shows
unpushed largefiles as missing) , resend it or it will be fixed in flight?

2016-06-24 9:47 GMT+02:00 liscju <piotr.listkiewicz@gmail.com>:

> # HG changeset patch
> # User liscju <piotr.listkiewicz@gmail.com>
> # Date 1466714237 -7200
> #      Thu Jun 23 22:37:17 2016 +0200
> # Node ID 2670e4331e0c763b04c0de2e1149917ccca42364
> # Parent  6d96658a22b0fc1eb210c99c5629dd69fedf3006
> largefiles: check file in the repo store before checking remotely
> (issue4686)
>
> Problem was files to check were gathered in the repository where
> the verify was launched but verification was done on the remote
> store. It was observed when user committed in cloned repository
> and ran verify before pushing - committed files were marked
> as non existing.
>
> This commit fixes this by checking in the remote store only files
> that are not existing in the repository store where verify was launched.
>
> Solution is similiar to fd288d118074
>
> diff --git a/hgext/largefiles/localstore.py
> b/hgext/largefiles/localstore.py
> --- a/hgext/largefiles/localstore.py
> +++ b/hgext/largefiles/localstore.py
> @@ -45,10 +45,23 @@ class localstore(basestore.basestore):
>          with open(path, 'rb') as fd:
>              return lfutil.copyandhash(fd, tmpfile)
>
> +    def _hashesavailablelocally(self, hashes):
> +        localhashes = [hash for hash in hashes
> +                       if lfutil.instore(self.repo, hash)]
> +        return localhashes
> +
>      def _verifyfiles(self, contents, filestocheck):
>          failed = False
> +        expectedhashes = [expectedhash
> +                          for cset, filename, expectedhash in
> filestocheck]
> +        localhashes = self._hashesavailablelocally(expectedhashes)
>          for cset, filename, expectedhash in filestocheck:
> -            storepath, exists = lfutil.findstorepath(self.remote,
> expectedhash)
> +            if expectedhash in localhashes:
> +                storepath, exists = lfutil.findstorepath(
> +                    self.repo, expectedhash)
> +            else:
> +                storepath, exists = lfutil.findstorepath(
> +                    self.remote, expectedhash)
>              if not exists:
>                  self.ui.warn(
>                      _('changeset %s: %s references missing %s\n')
> diff --git a/tests/test-largefiles-wireproto.t
> b/tests/test-largefiles-wireproto.t
> --- a/tests/test-largefiles-wireproto.t
> +++ b/tests/test-largefiles-wireproto.t
> @@ -149,6 +149,14 @@ largefiles clients refuse to push largef
>    $ hg commit -m "m2"
>    Invoking status precommit hook
>    A f2
> +  $ hg verify --large
> +  checking changesets
> +  checking manifests
> +  crosschecking files in changesets and manifests
> +  checking files
> +  2 files, 2 changesets, 2 total revisions
> +  searching 1 changesets for largefiles
> +  verified existence of 1 revisions of 1 largefiles
>    $ hg serve --config extensions.largefiles=! -R ../r6 -d -p $HGPORT
> --pid-file ../hg.pid
>    $ cat ../hg.pid >> $DAEMON_PIDS
>    $ hg push http://localhost:$HGPORT
> diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
> --- a/tests/test-largefiles.t
> +++ b/tests/test-largefiles.t
> @@ -1536,8 +1536,11 @@ revert some files to an older revision
>    searching 1 changesets for largefiles
>    verified existence of 3 revisions of 3 largefiles
>
> -- introduce missing blob in local store repo and make sure that this is
> caught:
> +- introduce missing blob in local store repo and remote store
> +and make sure that this is caught:
> +
>    $ mv $TESTTMP/d/.hg/largefiles/e166e74c7303192238d60af5a9c4ce9bef0b7928
> .
> +  $ rm .hg/largefiles/e166e74c7303192238d60af5a9c4ce9bef0b7928
>    $ hg verify --large
>    checking changesets
>    checking manifests
> @@ -1556,7 +1559,8 @@ revert some files to an older revision
>    [1]
>
>  - cleanup
> -  $ mv e166e74c7303192238d60af5a9c4ce9bef0b7928 $TESTTMP/d/.hg/largefiles/
> +  $ cp e166e74c7303192238d60af5a9c4ce9bef0b7928 $TESTTMP/d/.hg/largefiles/
> +  $ mv e166e74c7303192238d60af5a9c4ce9bef0b7928 .hg/largefiles/
>
>  - verifying all revisions will fail because we didn't clone all
> largefiles to d:
>    $ echo 'T-shirt' >
> $TESTTMP/d/.hg/largefiles/eb7338044dc27f9bc59b8dd5a246b065ead7a9c4
>

On Fri, 24 Jun 2016 09:47:16 +0200, liscju wrote:
> # HG changeset patch
> # User liscju <piotr.listkiewicz@gmail.com>
> # Date 1466714237 -7200
> #      Thu Jun 23 22:37:17 2016 +0200
> # Node ID 2670e4331e0c763b04c0de2e1149917ccca42364
> # Parent  6d96658a22b0fc1eb210c99c5629dd69fedf3006
> largefiles: check file in the repo store before checking remotely (issue4686)
> 
> Problem was files to check were gathered in the repository where
> the verify was launched but verification was done on the remote
> store. It was observed when user committed in cloned repository
> and ran verify before pushing - committed files were marked
> as non existing.
> 
> This commit fixes this by checking in the remote store only files
> that are not existing in the repository store where verify was launched.

I'm not pretty sure, but this patch seems good because the docstring says
"localstore first attempts to grab files out of the store in the remote",
which would imply in-store files precede remote files.

> --- a/hgext/largefiles/localstore.py
> +++ b/hgext/largefiles/localstore.py
> @@ -45,10 +45,23 @@ class localstore(basestore.basestore):
>          with open(path, 'rb') as fd:
>              return lfutil.copyandhash(fd, tmpfile)
>  
> +    def _hashesavailablelocally(self, hashes):
> +        localhashes = [hash for hash in hashes
> +                       if lfutil.instore(self.repo, hash)]
> +        return localhashes
> +
>      def _verifyfiles(self, contents, filestocheck):
>          failed = False
> +        expectedhashes = [expectedhash
> +                          for cset, filename, expectedhash in filestocheck]
> +        localhashes = self._hashesavailablelocally(expectedhashes)
>          for cset, filename, expectedhash in filestocheck:
> -            storepath, exists = lfutil.findstorepath(self.remote, expectedhash)
> +            if expectedhash in localhashes:
> +                storepath, exists = lfutil.findstorepath(
> +                    self.repo, expectedhash)
> +            else:
> +                storepath, exists = lfutil.findstorepath(
> +                    self.remote, expectedhash)

Is there any reason to build localhashes list?

I think "expectedhash in localhashes" can be simply written as
"lfutil.instore(self.repo, expectedhash)".

>
> Is there any reason to build localhashes list?
> I think "expectedhash in localhashes" can be simply written as
> "lfutil.instore(self.repo, expectedhash)".


There is no reason, this is changed in V2

2016-06-26 17:00 GMT+02:00 Yuya Nishihara <yuya@tcha.org>:

> On Fri, 24 Jun 2016 09:47:16 +0200, liscju wrote:
> > # HG changeset patch
> > # User liscju <piotr.listkiewicz@gmail.com>
> > # Date 1466714237 -7200
> > #      Thu Jun 23 22:37:17 2016 +0200
> > # Node ID 2670e4331e0c763b04c0de2e1149917ccca42364
> > # Parent  6d96658a22b0fc1eb210c99c5629dd69fedf3006
> > largefiles: check file in the repo store before checking remotely
> (issue4686)
> >
> > Problem was files to check were gathered in the repository where
> > the verify was launched but verification was done on the remote
> > store. It was observed when user committed in cloned repository
> > and ran verify before pushing - committed files were marked
> > as non existing.
> >
> > This commit fixes this by checking in the remote store only files
> > that are not existing in the repository store where verify was launched.
>
> I'm not pretty sure, but this patch seems good because the docstring says
> "localstore first attempts to grab files out of the store in the remote",
> which would imply in-store files precede remote files.
>
> > --- a/hgext/largefiles/localstore.py
> > +++ b/hgext/largefiles/localstore.py
> > @@ -45,10 +45,23 @@ class localstore(basestore.basestore):
> >          with open(path, 'rb') as fd:
> >              return lfutil.copyandhash(fd, tmpfile)
> >
> > +    def _hashesavailablelocally(self, hashes):
> > +        localhashes = [hash for hash in hashes
> > +                       if lfutil.instore(self.repo, hash)]
> > +        return localhashes
> > +
> >      def _verifyfiles(self, contents, filestocheck):
> >          failed = False
> > +        expectedhashes = [expectedhash
> > +                          for cset, filename, expectedhash in
> filestocheck]
> > +        localhashes = self._hashesavailablelocally(expectedhashes)
> >          for cset, filename, expectedhash in filestocheck:
> > -            storepath, exists = lfutil.findstorepath(self.remote,
> expectedhash)
> > +            if expectedhash in localhashes:
> > +                storepath, exists = lfutil.findstorepath(
> > +                    self.repo, expectedhash)
> > +            else:
> > +                storepath, exists = lfutil.findstorepath(
> > +                    self.remote, expectedhash)
>
> Is there any reason to build localhashes list?
>
> I think "expectedhash in localhashes" can be simply written as
> "lfutil.instore(self.repo, expectedhash)".
>