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

tooling for better copy detection #1529

Merged
merged 4 commits into from
Aug 20, 2024
Merged

tooling for better copy detection #1529

merged 4 commits into from
Aug 20, 2024

Conversation

Byron
Copy link
Owner

@Byron Byron commented Aug 19, 2024

A PR to add tooling and a test created with it to faciliate changes to the copy-detection algorithm.

Related to #1524

Tasks

  • royal-copy - degenerative copy from index with pathspecs
    • that away, the content of relevant files can be approximated and should still diff similarly
  • content-to-script - here-documents for all blobs of the listed trees
  • setup a fixture by manually creating indices of the respective tree, and commit them in order
  • create a tracked diff and assert on its renames (as they are now)

Documenting the current behaviour would allow changes to be expressed and then implemented.
Unfortuantely, for reasons of convenience, the test is on a higher level than the implementation has to be.

@Byron Byron force-pushed the better-copy-detection branch 2 times, most recently from 7251db7 to f67249b Compare August 19, 2024 19:54
…IDEs.

Given it's marginal effect on the binary size, the negative effects on RustRover
at least, and the added complexity, I think that ultimately it's not worth keeping,
unfortunately.
Its main purpose is to help build test-cases from real-world repositories.
@@ -378,6 +379,84 @@ mod track_rewrites {
Ok(())
}

#[test]
fn jj_realistic_needs_to_be_more_clever() -> crate::Result {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Due to the complexity of setting up the diffing in the tracker, I decided to add the test on gix level instead of on plumbing level, where everything is far more 'simulated'.

The benefit is that here one can define the final result exactly as one would want to see it, but with the downside that the implementation actually happens elsewhere, i.e. in the tracker, which has its own set of tests which should probably also be amended with boiled-down versions of this once the algorithm is adjusted.

One assume that that the algorithm changes will already change the outcome of the existing tests.

… shell script.

The shell-script will reproduce the repository, as long as the history is linear.
@Byron Byron marked this pull request as ready for review August 20, 2024 11:38
The pathspec list to reduce the set of files was generated with

```
git diff 47bd6f4aa4a7eeef8b01ce168c6c771bdfffcbd3~1 47bd6f4aa4a7eeef8b01ce168c6c771bdfffcbd3  --stat --no-renames --name-only
```

The assets and script to reproduce the fixture was created with:

```
cargo run -p internal-tools -- git-to-sh -c2 /Users/byron/dev/github.com/martinvonz/jj assets/jj-trackcopy-1 47bd6f4aa4a7eeef8b01ce168c6c771bdfffcbd3 CHANGELOG.md cli/src/commands/cat.rs cli/src/commands/chmod.rs cli/src/commands/file/chmod.rs cli/src/commands/file/mod.rs cli/src/commands/file/print.rs cli/src/commands/mod.rs cli/tests/cli-reference@.md.snap cli/tests/runner.rs cli/tests/test_acls.rs cli/tests/test_cat_command.rs cli/tests/test_chmod_command.rs cli/tests/test_diffedit_command.rs cli/tests/test_file_chmod_command.rs cli/tests/test_file_print_command.rs cli/tests/test_fix_command.rs cli/tests/test_global_opts.rs cli/tests/test_immutable_commits.rs cli/tests/test_move_command.rs cli/tests/test_new_command.rs cli/tests/test_squash_command.rs cli/tests/test_unsquash_command.rs
```

It's notable that such issues are currently expected as the tracker
implementation isn't as precise as the one in Git, which tracks more
than one candidate.
@Byron Byron merged commit 7b7902e into main Aug 20, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant