Page MenuHomePhabricator

rhg: handle null changelog and manifest revisions
ClosedPublic

Authored by aalekseyev on Tue, Oct 12, 2:44 PM.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aalekseyev created this revision.Tue, Oct 12, 2:44 PM
martinvonz added inline comments.
tests/test-empty-manifest-index.t
7–11

FYI, this can be written hg commit -m "init" --config ui.allowemptycommit=true

aalekseyev added inline comments.Tue, Oct 12, 3:20 PM
tests/test-empty-manifest-index.t
7–11

Thanks, I did not know that was supported in vanilla hg.
However, this is not equivalent: --amend produces an empty 00manifest.i file, while ui.allowemptycommit=true produces an absent file.
In fact the fixes in this rev are sufficient for the amend case, but are not sufficient for the emptycommit case (test is broken with rhg).
I'm working on further changes that make rhg work in an empty repo and an empty commit.

martinvonz accepted this revision.Tue, Oct 12, 3:53 PM
martinvonz added inline comments.
tests/test-empty-manifest-index.t
7–11

Makes sense about the difference.

This revision is now accepted and ready to land.Tue, Oct 12, 3:53 PM
This revision was automatically updated to reflect the committed changes.

@martinvonz, thanks!
I pushed the follow-up changes in:
https://phab.mercurial-scm.org/D11651

This broke a Rust unit test: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/253010#L32

You can run these tests with (cd rust && cargo test). Could you send a follow-up patch to fix this?

This broke a Rust unit test: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/253010#L32
You can run these tests with (cd rust && cargo test). Could you send a follow-up patch to fix this?

Sorry, I didn't even realize those tests existed. I only tested with tests/run-tests.py --rhg. Thanks.
I'll do that.

I'll do that.

Submitted: https://phab.mercurial-scm.org/D11656
Is that alright, or do you prefer me amend the original commit? (will I be able to phabsend it though?)

I'll do that.

Submitted: https://phab.mercurial-scm.org/D11656
Is that alright, or do you prefer me amend the original commit? (will I be able to phabsend it though?)

I don't think you can, but I can fold that into your original commit.

This breaks the CI: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254045

It seems that the previous assumption made in the code was that null revs were to be shortcut and this assumption was not changed everywhere. @aalekseyev The easiest way of making sure you're not breaking anything is to push a topic to https://foss.heptapod.net/mercurial/mercurial-devel (if you create an account I can give you the rights to do so).

Right now other tests are failing, so you might see unrelated failures, but I'm looking into those as well. I try to keep the CI is mostly green, but I was on vacation.

tests/test-empty-manifest-index.t
1

Does the test suite already have a test for empty manifest or is this specific to rhg? I'm asking because I don't think mixing rhg config options within "normal tests" is a good idea. There is test-rhg.t that is specifically done to test the differences (or lack thereof) and the fallback mechanism.

This breaks the CI: https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/254045
It seems that the previous assumption made in the code was that null revs were to be shortcut and this assumption was not changed everywhere. @aalekseyev The easiest way of making sure you're not breaking anything is to push a topic to https://foss.heptapod.net/mercurial/mercurial-devel (if you create an account I can give you the rights to do so).
Right now other tests are failing, so you might see unrelated failures, but I'm looking into those as well. I try to keep the CI is mostly green, but I was on vacation.

Sorry for this breakage. I've been running tests by hand, but apparently forgot to run all of them for that patch.
This should be fixed by 62630772f5f7 (https://phab.mercurial-scm.org/D11664).

aalekseyev added inline comments.Mon, Oct 18, 5:27 AM
tests/test-empty-manifest-index.t
1

I couldn't find an existing test for null manifest revision.
I assumed that if one existed, it would have caught the rhg issue, therefore there is none.
It's possible that the test exists but opts-out rhg, and I had a quick look through rhg opt-outs, and found nothing relevant.

mixing rhg config options within "normal tests"

Oh, yeah, I added those temporarily to make sure I was testing what I thought I was testing.
I removed those config options in the very next commit (9d0e5629cfbf).

Alphare added inline comments.Mon, Oct 18, 5:34 AM
tests/test-empty-manifest-index.t
1

I couldn't find an existing test for null manifest revision.

Sure, me neither, that's fine. :)

Oh, yeah, I added those temporarily to make sure I was testing what I thought I was testing.
I removed those config options in the very next commit

I would have preferred not to have those at all and to have waiting for the fix, but I guess the churn is not worth it anymore. Thanks for fixing the issues, I'm trying to catch up to what happened. :)

... push a topic to https://foss.heptapod.net/mercurial/mercurial-devel (if you create an account I can give you the rights to do so).

I made an account with username "aalekseyev". Can you please give me those rights?
Thanks.