This is an archive of the discontinued Mercurial Phabricator instance.

rust-revlog: move check for nodemap requirement to caller
Needs ReviewPublic

Authored by martinvonz on Wed, Apr 13, 9:10 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Summary

It's good for both making Revlog testable and reusable to have it
not depend on the higher-level Repo type. This patch is one step in
towards that. Additionally, this change in particular gives the
callers more control over when to use a nodemap.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Wed, Apr 13, 9:10 AM
Alphare requested changes to this revision.Thu, Apr 14, 5:03 AM
Alphare added a subscriber: Alphare.

While I think you're right about separating the revlogs from the repo, the repeated logic should probably be factored into a function somewhere. Why not in nodemap.rs?

This revision now requires changes to proceed.Thu, Apr 14, 5:03 AM

While I think you're right about separating the revlogs from the repo, the repeated logic should probably be factored into a function somewhere. Why not in nodemap.rs?

Two things:

  • Do we want to use persistent nodemaps for all types of revlogs? This was what I meant by "Additionally, this change in particular gives the callers more control over when to use a nodemap.". Looking at the Python code, we don't seem to use it for filelogs. I'll add a patch to this series to stop using persistent nodemap for filelogs.
  • I was thinking of moving the conditions further up the stack, to the repo level. If we do that, then the repo might be a natural place to extract a function. Or maybe I'll put it in nodemap.rs as you said. I'll update this patch a bit later.

Two things:

  • Do we want to use persistent nodemaps for all types of revlogs? This was what I meant by "Additionally, this change in particular gives the callers more control over when to use a nodemap.". Looking at the Python code, we don't seem to use it for filelogs. I'll add a patch to this series to stop using persistent nodemap for filelogs.

We will probably only want it for changelogs and manifestlogs. Thanks for sending the patch for the filelogs. Maybe that'll change in the future (with unified revlog?), but that can always be added.

  • I was thinking of moving the conditions further up the stack, to the repo level. If we do that, then the repo might be a natural place to extract a function. Or maybe I'll put it in nodemap.rs as you said. I'll update this patch a bit later.

Either seem fine to me.

  • I was thinking of moving the conditions further up the stack, to the repo level. If we do that, then the repo might be a natural place to extract a function. Or maybe I'll put it in nodemap.rs as you said. I'll update this patch a bit later.

Either seem fine to me.

I moved it to Repo in D12562.