creating new wc commit when @ becomes immutable

[tdaron]

Hi !
I am currently trying to write a fix for #3201

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have added tests to cover my changes

[tdaron]

I currently have this implementation but it starts an infinite loop creating commits again and again on top of the @ immutable, cause when i call tx.finish() to end-up my new wc commit creation, the maybe_new_wc_commit still points to @ and not on my newly created commit (which should have become @ but doesn't)

am i doing a huge blunder ?

[tdaron]

Is it any good ?

I tried to avoid copy-pasting check_rewritable so i tried organizing stuff in a clear way.

[martinvonz]

This is still marked as a draft. Did you just forget to mark it ready?

[tdaron]

Nope I didn't add any tests yet, but I wanted a feedback on my implementation to avoid writing any additional code that I should delete anyway

Sry if I shouldn't ask review for that I don't really know how things works, I am new to contributing haha

[martinvonz]

Nope I didn't add any tests yet, but I wanted a feedback on my implementation to avoid writing any additional code that I should delete anyway

Sry if I shouldn't ask review for that I don't really know how things works, I am new to contributing haha

Ah, that makes sense. I think people typically mark it ready for review and leave a comment saying that they haven't added tests yet. I generally ignore PRs in draft state. I don't know if others do the same.

[martinvonz]

Nope I didn't add any tests yet, but I wanted a feedback on my implementation to avoid writing any additional code that I should delete anyway

Actually, the tests would be end-to-end tests of the CLI, I think, so they should not have to depend on how exactly it's implemented. https://github.com/martinvonz/jj/blob/main/cli/tests/test_immutable_commits.rs is probably a good place to add them.

[martinvonz]

Thanks for working on this! Sorry it took me a while to get to it.

[tdaron]

No problem haha, thank you to take time to review my PR :)

[tdaron]

I added a test, and i am now checking all workspaces to update them.

I also inlined what i could, but i didn't moved my check to maybe_snapshot because I didn't know what i had to look at as the transaction has not even been created at this point. But I might just misunderstood this part of the code.

Shall i add a test to check if it updates well every workspace ?

[tdaron]

I added more tests, i had two issues doing it:

• tx.mut_repo().view().wc_commit_ids() item's order is not deterministic, so i had to sort them to be able to test the output. (otherwise the order of the warnings could change)
• This one is a question, but am i supposed to create a different new wc commit per workspace (this is how it works now) or create one and check out all affected workspaces on the same newly created commit ?

[martinvonz]

I added more tests, i had two issues doing it:

• tx.mut_repo().view().wc_commit_ids() item's order is not deterministic, so i had to sort them to be able to test the output. (otherwise the order of the warnings could change)

Makes sense.

• This one is a question, but am i supposed to create a different new wc commit per workspace (this is how it works now) or create one and check out all affected workspaces on the same newly created commit ?

It's rare that multiple workspaces point to the same commit, so I don't think we need to worry about that. I.e., what you have right now is good enough IMO.

[tdaron]

In that case i think we have an usable version of the PR :)

Is there anything more i can do about it ?

[martinvonz]

Looks good. Thanks!