[PATCH 4 of 8] upgrade: introduce a _copyrevlog method

[Pierre-Yves David]

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 at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>> 
> wrote:
> 
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david at octobus.net
>     <mailto:pierre-yves.david at 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.


-- 
Pierre-Yves David