This is an archive of the discontinued Mercurial Phabricator instance.

rust-revlog: add methods for getting parent revs and entries
ClosedPublic

Authored by martinvonz on Apr 5 2022, 3:08 PM.

Diff Detail

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

Event Timeline

martinvonz created this revision.Apr 5 2022, 3:08 PM

FYI, the functions here may seem arbitrary, but they're enough for a very simple Mercurial backend for my 20%/hobby project (see commit https://github.com/martinvonz/jj/commit/hg if you're curious). I'll add tests to this patch if you (mostly @Raphael, I suppose) think the changes look good.

Alphare added a subscriber: Alphare.Apr 6 2022, 8:48 AM
Alphare added inline comments.
rust/hg-core/src/revlog/changelog.rs
33–42

Docstring needs adjustment

rust/hg-core/src/revlog/revlog.rs
346

Mhhh I wonder if pN_entry might be a better name, then pN_rev would just be pN. That way it's more obvious you're getting an entry?

martinvonz updated this revision to Diff 32868.Apr 7 2022, 12:24 PM
martinvonz updated this revision to Diff 32869.Apr 7 2022, 12:27 PM
martinvonz updated this revision to Diff 32989.Apr 8 2022, 6:10 PM
martinvonz retitled this revision from [RFC] rhg: implement more methods on revlogs to [RFC] rust-revlog: implement more methods on revlogs.Apr 9 2022, 1:36 AM
martinvonz updated this revision to Diff 32990.
Alphare accepted this revision.Apr 11 2022, 6:23 AM
This revision is now accepted and ready to land.Apr 11 2022, 6:23 AM
martinvonz retitled this revision from [RFC] rust-revlog: implement more methods on revlogs to rust-revlog: add methods for getting parent revs and entries.Apr 11 2022, 12:07 PM
martinvonz updated this revision to Diff 33004.

FYI, the functions here may seem arbitrary, but they're enough for a very simple Mercurial backend for my 20%/hobby project (see commit https://github.com/martinvonz/jj/commit/hg if you're curious). I'll add tests to this patch if you (mostly @Raphael, I suppose) think the changes look good.

I just removed the "[RFC]" without adding tests :) But I can add tests if you want. Where's a good place to add them in that case?

I think we'll want to write some basic unit tests at the end of the file. We haven't been very good about doing these in the Rust code base, but I was starting again recently with the dirstatemap (and found some bugs, of course), so that's probably a good idea.

I think we'll want to write some basic unit tests at the end of the file. We haven't been very good about doing these in the Rust code base, but I was starting again recently with the dirstatemap (and found some bugs, of course), so that's probably a good idea.

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance. What do you think about taking this without tests now and then I add tests when I send that other series? Or, if you think that refactoring sounds like a good idea, I can clean that series up and insert it before this patch.

Hopefully the patches before this one can be queued without it (though there's no real rush for us yet).

By the way, Repo is not currently Send. I think the issue was with LazyCell. Do you think we should do something about that or does it not matter for your plans?

martinvonz updated this revision to Diff 33138.Apr 12 2022, 2:33 PM

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance.

I see no issue for doing this, it's probably a good idea.

What do you think about taking this without tests now and then I add tests when I send that other series? Or, if you think that refactoring sounds like a good idea, I can clean that series up and insert it before this patch.

I prefer the option where you do the clean up first of course, but the follow-up works just as well as long as it's done.

By the way, Repo is not currently Send. I think the issue was with LazyCell. Do you think we should do something about that or does it not matter for your plans?

I'm not sure I see the value of repo being Send for now. It being Sync should probably be good enough for all of our purposes? Did you have something in mind?

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance.

I see no issue for doing this, it's probably a good idea.

I've inserted those patches in this series.

What do you think about taking this without tests now and then I add tests when I send that other series? Or, if you think that refactoring sounds like a good idea, I can clean that series up and insert it before this patch.

I prefer the option where you do the clean up first of course, but the follow-up works just as well as long as it's done.

While trying to add tests for that last night I realized that writing the test revlogs gets pretty verbose. I then found the IndexEntryBuilder in index.rs. Maybe we should move that to a common place? Maybe even out of test code so it can be used when we start writing revlogs from Rust?

By the way, Repo is not currently Send. I think the issue was with LazyCell. Do you think we should do something about that or does it not matter for your plans?

I'm not sure I see the value of repo being Send for now. It being Sync should probably be good enough for all of our purposes? Did you have something in mind?

Yes, what hg would want is Sync. The context was that I needed the hg backend to be Sync for my 20% project so I just wrapped the hg Repo in a mutex, which means that Repo needed to be Send. Anyway, if you agree that hg wants it to eventually be Sync, we'll need to address those LazyCells.

That's actually related to another series I had not sent yet, which is about removing the &Repo input to Revlog::open(). It would be much easier to write tests if we didn't have to create a Repo instance.

I see no issue for doing this, it's probably a good idea.

I've inserted those patches in this series.

I didn't notice you had queued some patches in this series already (thanks!). I've rebased the added patches on top of @ now.