Patchwork [4,of,8] upgrade: introduce a _copyrevlog method

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1564509524 -7200
#      Tue Jul 30 19:58:44 2019 +0200
# Node ID 71edc0b44b7108cddee33d99f0ec586a0b0405c9
# Parent  aa19f478cfdfe781acc15cd26339644757156355
# EXP-Topic upgrade-select
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 71edc0b44b71
upgrade: introduce a _copyrevlog method

This function copies a revlog from the old store to the new one, without
re-adding all deltas manually. This will eventually save a lot of time when some
revlog does not needs any conversions.

Code actually using this will be introduced in later changesets.

On Mon, Aug 5, 2019 at 9:55 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1564509524 -7200
> #      Tue Jul 30 19:58:44 2019 +0200
> # Node ID 71edc0b44b7108cddee33d99f0ec586a0b0405c9
> # Parent  aa19f478cfdfe781acc15cd26339644757156355
> # EXP-Topic upgrade-select
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> 71edc0b44b71
> upgrade: introduce a _copyrevlog method
>
> This function copies a revlog from the old store to the new one, without
> re-adding all deltas manually. This will eventually save a lot of time
> when some
> revlog does not needs any conversions.
>
> Code actually using this will be introduced in later changesets.
>
> diff --git a/mercurial/upgrade.py b/mercurial/upgrade.py
> --- a/mercurial/upgrade.py
> +++ b/mercurial/upgrade.py
> @@ -533,6 +533,35 @@ def _revlogfrompath(repo, path):
>          #reverse of "/".join(("data", path + ".i"))
>          return filelog.filelog(repo.svfs, path[5:-2])
>
> +def _copyrevlog(tr, destrepo, oldrl, unencodedname):
> +    """copy all relevant files for `oldrl` into `destrepo` store
>

I'd like to point out that low-level code like this is a violation of
storage abstractions. In this case, the upgrade code is making assumptions
about which revlogs exist and what they are named. The proper way to
implement this feature is to implement a `copystorage()` method or some
such on the per-primitive storage interfaces (changelog, manifestlog,
filelog, etc) and to have the upgrade code call it when copying between
compatible repo types.

That being said, the storage interfaces aren't fully flushed out yet and my
recollection is the upgrade code is full of cases that assume revlogs. So
it is probably acceptable to take this technical debt until we can devise a
way to better handle repo upgrades in the storage interfaces.


> +
> +    Files are copied "as is" without any transformation. The copy is
> performed
> +    without extra checks. Callers are responsible for making sure the
> copied
> +    content is compatible with format of the destination repository.
> +    """
> +    oldrl = getattr(oldrl, '_revlog', oldrl)
> +    newrl = _revlogfrompath(destrepo, unencodedname)
> +    newrl = getattr(newrl, '_revlog', newrl)
> +
> +    oldvfs = oldrl.opener
> +    newvfs = newrl.opener
> +    oldindex = oldvfs.join(oldrl.indexfile)
> +    newindex = newvfs.join(newrl.indexfile)
> +    olddata = oldvfs.join(oldrl.datafile)
> +    newdata = newvfs.join(newrl.datafile)
> +
> +    newdir = newvfs.dirname(newrl.indexfile)
> +    newvfs.makedirs(newdir)
> +
> +    util.copyfile(oldindex, newindex)
> +    if oldrl.opener.exists(olddata):
> +        util.copyfile(olddata, newdata)
> +
> +    if not (unencodedname.endswith('00changelog.i')
> +            or unencodedname.endswith('00manifest.i')):
> +        destrepo.svfs.fncache.add(unencodedname)
> +
>  def _clonerevlogs(ui, srcrepo, dstrepo, tr, deltareuse,
> forcedeltabothparents):
>      """Copy revlogs between 2 repos."""
>      revcount = 0
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

On 8/6/19 4:33 AM, Gregory Szorc wrote:
> On Mon, Aug 5, 2019 at 9:55 AM Pierre-Yves David 
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> 
> wrote:
> 
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>>
>     # Date 1564509524 -7200
>     #      Tue Jul 30 19:58:44 2019 +0200
>     # Node ID 71edc0b44b7108cddee33d99f0ec586a0b0405c9
>     # Parent  aa19f478cfdfe781acc15cd26339644757156355
>     # EXP-Topic upgrade-select
>     # Available At https://bitbucket.org/octobus/mercurial-devel/
>     #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 71edc0b44b71
>     upgrade: introduce a _copyrevlog method
> 
>     This function copies a revlog from the old store to the new one, without
>     re-adding all deltas manually. This will eventually save a lot of
>     time when some
>     revlog does not needs any conversions.
> 
>     Code actually using this will be introduced in later changesets.
> 
>     diff --git a/mercurial/upgrade.py b/mercurial/upgrade.py
>     --- a/mercurial/upgrade.py
>     +++ b/mercurial/upgrade.py
>     @@ -533,6 +533,35 @@ def _revlogfrompath(repo, path):
>               #reverse of "/".join(("data", path + ".i"))
>               return filelog.filelog(repo.svfs, path[5:-2])
> 
>     +def _copyrevlog(tr, destrepo, oldrl, unencodedname):
>     +    """copy all relevant files for `oldrl` into `destrepo` store
> 
> 
> I'd like to point out that low-level code like this is a violation of 
> storage abstractions. In this case, the upgrade code is making 
> assumptions about which revlogs exist and what they are named. The 
> proper way to implement this feature is to implement a `copystorage()` 
> method or some such on the per-primitive storage interfaces (changelog, 
> manifestlog, filelog, etc) and to have the upgrade code call it when 
> copying between compatible repo types.

I tried that route first, but I lacked access to a couple of important 
bit at the storage layer. So I took a simpler approach. (Not saying it 
is impossible, but that was not trivial when I tried)

> That being said, the storage interfaces aren't fully flushed out yet and 
> my recollection is the upgrade code is full of cases that assume 
> revlogs. So it is probably acceptable to take this technical debt until 
> we can devise a way to better handle repo upgrades in the storage 
> interfaces.

Yeah, the upgrade code is already assuming revlog all over the place, so 
it seems fine to add another one. For example `_revlogfrompath` has a 
similar logic than the matchrevlog function.