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.

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

Crash if a dir jj doesn't track is a file in another commit. Also happens if untracked file conflicts with a tracked file. #976

Closed
ilyagr opened this issue Jan 5, 2023 · 3 comments
Labels
🐛bug Something isn't working good first issue Good for newcomers

Comments

@ilyagr
Copy link
Contributor

ilyagr commented Jan 5, 2023

Here is a part of a test that crashes (add it to test_resolve_command.rs):

#[test]
fn test_crash() {
    let test_env = TestEnvironment::default();
    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
    let repo_path = test_env.env_root().join("repo");

    create_commit(&test_env, &repo_path, "base", &[], &[("file", "base\n")]);
    create_commit(&test_env, &repo_path, "dir", &["base"], &[]);
    std::fs::remove_file(repo_path.join("file")).unwrap();
    std::fs::create_dir(repo_path.join("file")).unwrap();

    // Uncommenting the line below fixes the crash
    // std::fs::write(repo_path.join("file").join("placeholder"), "").unwrap();
    // Crash happens here
    create_commit(&test_env, &repo_path, "del", &["base"], &[]);
    std::fs::remove_file(repo_path.join("file")).unwrap();
}

The point is that file is a directory that jj doesn't track since it's empty. In another commit, it is a file. Updating to the second commit confuses jj.

The error is:

thread 'test_crazy_conflict_description' panicked at 'Unexpected failure.
code-255
stderr=```"BUG: Working copy lock was dropped without being closed.\nInternal error: Failed to check out commit 0e5547529a736970e82066471c16ff3bec6ae23b: Failed to open file /tmp/jj-test-aMpIzb/repo/file for writing: Os { code: 17, kind: AlreadyExists, message: \"File exists\" }\n"```
command=`"/home/ilyagr/dev/jj/target/debug/jj" "new" "-m" "del" "base"`
code=255
stdout=""
stderr="BUG: Working copy lock was dropped without being closed.\nInternal error: Failed to check out commit 0e5547529a736970e82066471c16ff3bec6ae23b: Failed to open file /tmp/jj-test-aMpIzb/repo/file for writing: Os { code: 17, kind: AlreadyExists, message: \"File exists\" }\n"
', /rustc/352eb59a4c33abf739914422f2ad975925750146/library/core/src/ops/function.rs:507:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 5, 2023

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 5, 2023

One solution would be to check if anything happens to be in the way around

https://github.com/martinvonz/jj/blob/af55d17a25ad9172bbcb1d222ac2e291df0ed650/lib/src/working_copy.rs#L896-L900

We could then remove it. I'm unsure if that's safe

In other news, when I reproduced this bug in my own repo, I got a bit stuck until I removed the offending directory:

$ rm Cargo.toml && mkdir Cargo.toml
$ RUST_BACKTRACE=1 target/debug/jj up @-
BUG: Working copy lock was dropped without being closed.
Internal error: Failed to check out commit 75eddb01c4150fd349a5ce1f4b5ece718940a281: Failed to open file /home/ilyagr/dev/jj/Cargo.toml for writing: Os { code: 17, kind: AlreadyExists, message: "File exists" }

$ JJ_BACKTRACE=1 target/debug/jj up @-
Error: The working copy is stale (not updated since operation 8508b3dece63).
Hint: Run `jj workspace update-stale` to update it.

$ jj workspace update-stale
BUG: Working copy lock was dropped without being closed.
Internal error: Failed to check out commit 75eddb01c4150fd349a5ce1f4b5ece718940a281: Failed to open file /home/ilyagr/dev/jj/Cargo.toml for writing: Os { code: 17, kind: AlreadyExists, message: "File exists" }

@martinvonz
Copy link
Member

I suppose we could just delete the directory if it's empty in this scenario, but it can be more complicated if the directory was instead ignored. I think we should return a list of paths and errors we encountered, so we can tell the user that we failed to update some paths. Maybe we can just leave those paths as they are in the working copy. That would mean that they would instead appear modified in the working copy. I'm not sure if there's a better option.

The "Working copy lock was dropped without being closed." can probably be ignored. I think that just happened because of the other bug, and it only happens in debug builds.

@ilyagr ilyagr changed the title Crash if a dir jj doesn't track is a file in another commit Crash if a dir jj doesn't track is a file in another commit. Also happens if untracked file conflicts with a tracked file. Jan 21, 2023
@martinvonz martinvonz added 🐛bug Something isn't working good first issue Good for newcomers labels Mar 5, 2023
martinvonz added a commit that referenced this issue Oct 7, 2023
Before this patch, when updating to a commit that has a file that's
currently an ignored file on disk, jj would crash. After this patch,
we instead leave the conflicting files or directories on disk. We
print a helpful message about how to inspect the differences between
the intended working copy and the actual working copy, and how to
discard the unintended changes.

Closes #976.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants