Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cli: don't abandon non-discardable old wc commit by import_git_head() #3749

Merged
merged 1 commit into from
May 25, 2024
Merged

cli: don't abandon non-discardable old wc commit by import_git_head() #3749

merged 1 commit into from
May 25, 2024

Conversation

yuja
Copy link
Collaborator

@yuja yuja commented May 24, 2024

#3747

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@yuja
cli: don't abandon non-discardable old wc commit by import_git_head()
5a6e0c1 
Perhaps, the original intent was to abandon non-empty working-copy commit
assuming it was rewritten by git (therefore it should be superseded by the
current working-copy content.) However, there are other reasons that could
make the HEAD out of sync (including concurrent jj operations), and abandoning
non-empty commit can be a disaster. This patch turns it to safer side, and let
user abandon non-empty commit manually.

Fixes jj-vcs#3747
bnjmnt4n
bnjmnt4n approved these changes May 24, 2024
View reviewed changes
Copy link
Collaborator

@bnjmnt4n bnjmnt4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I can't think of any downsides to this change. Hopefully it makes the colocation experience much better.

@ilyagr
Copy link
Collaborator

ilyagr commented May 24, 2024 •
edited
Loading

(Non-blocking)

I tried out this PR in my copy of jj and ran into the bug again. With this PR, the bug created a single divergent commit instead of the mess it made before, so I think the PR definitely improved the outcome quite a bit. In fact, the only remaining problems in this example seems to be that the working copy and its parent commit were not abandoned. To be slightly more precise, I think going from the resulting state and to the desired state is a matter of jj abandon @ @-; jj next.

This time, the offending operation is a squash instead of a rebase.

Here's the repo (50MB): https://ilya.gr/share/jj-bug-2024-05-24.tar.gz

I definitely agree with the direction of this PR -- the "best" fix for the bug would be that the rules of the resolution of concurrent operation be such that any partial git import from a concurrent operation cleanly merges with the main operation and does not affect the outcome. This is easier said than done, of course.

@yuja
Copy link
Collaborator Author

yuja commented May 25, 2024

With this PR, the bug created a single divergent commit instead of the mess it made before,

I haven't inspected the op log, but I think that's because an intermediate HEAD state was captured by background process and checked out (i.e. created independent wc commits.) I don't think it can be "fixed" without introducing wider lock.

Maybe diverged wc pointers could be merged somehow, but I have no idea if that will work.

@yuja yuja merged commit 4478055 into jj-vcs:main May 25, 2024
16 checks passed
@yuja yuja deleted the push-rvqnpxrolmxm branch May 25, 2024 01:29
@yuja
Copy link
Collaborator Author

yuja commented May 25, 2024

fwiw, for background viewer processes, #2562 (comment) might help.

@yuja yuja mentioned this pull request May 25, 2024
Open
@yuja
Copy link
Collaborator Author

yuja commented May 25, 2024

Here's the repo (50MB): https://ilya.gr/share/jj-bug-2024-05-24.tar.gz

Filed as #3754

@ilyagr
Copy link
Collaborator

ilyagr commented May 25, 2024

fwiw, for background viewer processes, #2562 (comment) might help.

Very good point. We could even have an option to forbid more than one instance of jj from running without the --do-not-commit-transaction option, though this would go against lofty concurrency goals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnjmnt4n bnjmnt4n bnjmnt4n approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuja @ilyagr @bnjmnt4n