copy-tracking: plumb copy tracking through diff detection #4118

fowles · 2024-07-18T16:34:30Z

Re-approach copy tracking with a simpler signature. Push through end-to-end copy for diff -s.

This is not ready to submit, but is complete enough that I would like feedback on the direction. Remaining things to do:

update design doc
update CHANGELOG.md
plumb through all other diff commands
update the documentation?
update the config schema? (cli/src/config-schema.json)

yuja

I can't comment on copy-tracing API changes, so just about diff_util part.

I think it's good to use Commit or [Commit] in place of MergedTree. I'm trying to implement commit description templates, and show_diff() callers in description_util.rs will be switched to be using a Commit object. It may be either a temporary Commit physically written to the store, or in-memory Commit with fake commit id and physically-stored tree and parents.

cli/src/diff_util.rs

fowles · 2024-07-19T17:51:51Z

I can't comment on copy-tracing API changes, so just about diff_util part.

I think it's good to use Commit or [Commit] in place of MergedTree. I'm trying to implement commit description templates, and show_diff() callers in description_util.rs will be switched to be using a Commit object. It may be either a temporary Commit physically written to the store, or in-memory Commit with fake commit id and physically-stored tree and parents.

This is great! That will fit together much better.

martinvonz

Just one little comment for now. I'll have to review it in detail later

cli/examples/custom-backend/main.rs

PhilipMetzger

The things I mentioned in Discord.

cli/src/diff_util.rs

lib/src/merged_tree.rs

cli/src/merge_tools/diff_working_copies.rs

cli/src/diff_util.rs

docs/design/copy-tracking.md

lib/src/backend.rs

CHANGELOG.md

cli/src/commands/diff.rs

cli/examples/custom-backend/main.rs

cli/src/commands/diff.rs

lib/src/backend.rs

lib/tests/test_merged_tree.rs

lib/src/merged_tree.rs

cli/src/diff_util.rs

lib/src/merged_tree.rs

lib/src/backend.rs

cli/src/commands/diff.rs

cli/src/merge_tools/diff_working_copies.rs

cli/src/diff_util.rs

lib/src/conflicts.rs

cli/src/diff_util.rs

lib/src/merged_tree.rs

cli/src/diff_util.rs

lib/src/merged_tree.rs

cli/src/commands/diff.rs

martinvonz

Looks good to me, minus the few remaining nits.

But please give @yuja another chance to comment too.

@yuja: As I said elsewhere on this PR, I don't think the API changes in the first commit are quite right. However, I think the rest of the series looks correct, and it provides practical improvements for Git users. I therefore think it makes sense to get this in in its current form, even if we know we're going to have change the API later. I don't think this PR makes it significantly harder to make those changes later, whatever they are - the changes to the diff iterator/stream and the commands using them see like they probably won't change significantly. Let us know if you disagree, of course.

cli/src/diff_util.rs

martinvonz · 2024-08-10T05:33:57Z

And thanks a lot for your hard work and patience with this PR, @fowles!

cli/src/commands/debug/copy_detection.rs

cli/src/commands/diff.rs

lib/src/merged_tree.rs

cli/src/diff_util.rs

cli/src/commit_templater.rs

lib/src/backend.rs

cli/src/commands/diff.rs

PhilipMetzger

LGTM

cli/src/commands/fix.rs

cli/src/diff_util.rs

lib/src/merged_tree.rs

CHANGELOG.md

lib/src/backend.rs

- use a single commit instead of an array of them. This simplifies the implementation. A higher level api can wrap this when an array of commits is desired and those semantics are figured out. - since this API is directly 1-1 on parents, there are no conflicts - if we introduce a higher level API that handles lists of commits, we may need to restore the conflict/resolved distinction, but for now simplify

- add the field and make it compile, but don't use it yet

- force each diff command to explicitly enable copy tracking - enable copy tracking in diff_summary - post-process for diff iterator - post-process for diff stream - update changelog

fowles requested review from martinvonz and ilyagr July 18, 2024 16:34

fowles force-pushed the simplify branch 2 times, most recently from 9b49da6 to 6734640 Compare July 18, 2024 16:53

yuja reviewed Jul 19, 2024

View reviewed changes

cli/src/diff_util.rs Outdated Show resolved Hide resolved

fowles force-pushed the simplify branch 6 times, most recently from b7e16c4 to 3e59437 Compare August 7, 2024 01:43

PhilipMetzger marked this pull request as ready for review August 7, 2024 15:33

martinvonz reviewed Aug 7, 2024

View reviewed changes

cli/examples/custom-backend/main.rs Outdated Show resolved Hide resolved

fowles force-pushed the simplify branch 3 times, most recently from 76bac8e to 2f7fbd7 Compare August 7, 2024 17:58

PhilipMetzger reviewed Aug 7, 2024

View reviewed changes

fowles force-pushed the simplify branch 6 times, most recently from 7390d9a to f298f37 Compare August 9, 2024 02:13

ilyagr mentioned this pull request Aug 9, 2024

copy-tracking: improve --summary and add --stat #4245

Merged

4 tasks

martinvonz reviewed Aug 9, 2024

View reviewed changes

yuja reviewed Aug 9, 2024

View reviewed changes

lib/src/backend.rs Outdated Show resolved Hide resolved

cli/src/commands/diff.rs Outdated Show resolved Hide resolved

fowles force-pushed the simplify branch 3 times, most recently from d988f3f to c855ac8 Compare August 9, 2024 17:48

fowles force-pushed the simplify branch from c855ac8 to f694936 Compare August 9, 2024 18:40

martinvonz reviewed Aug 9, 2024

View reviewed changes

fowles force-pushed the simplify branch 2 times, most recently from d31bfee to 2baa663 Compare August 9, 2024 21:55

martinvonz reviewed Aug 9, 2024

View reviewed changes

lib/src/merged_tree.rs Outdated Show resolved Hide resolved

lib/src/merged_tree.rs Outdated Show resolved Hide resolved

fowles force-pushed the simplify branch 4 times, most recently from a087d90 to 64aec51 Compare August 10, 2024 02:18

martinvonz reviewed Aug 10, 2024

View reviewed changes

cli/src/commands/diff.rs Outdated Show resolved Hide resolved

cli/src/commands/diff.rs Outdated Show resolved Hide resolved

martinvonz approved these changes Aug 10, 2024

View reviewed changes

cli/src/diff_util.rs Outdated Show resolved Hide resolved

cli/src/diff_util.rs Outdated Show resolved Hide resolved

yuja approved these changes Aug 10, 2024

View reviewed changes

PhilipMetzger approved these changes Aug 10, 2024

View reviewed changes

cli/src/commands/fix.rs Show resolved Hide resolved

cli/src/diff_util.rs Show resolved Hide resolved

lib/src/merged_tree.rs Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

lib/src/backend.rs Outdated Show resolved Hide resolved

fowles added 8 commits August 11, 2024 16:47

copy-tracking: create CopyRecordMap and add it to diff summaries

7cc8b49

copy-tracking: create an explicit TreeDiffEntry struct

91bbc3f

copy-tracking: add source field to TreeDiffEntry

3be66be

- add the field and make it compile, but don't use it yet

copy-tracking: create a MaterializedTreeDiffEntry type

6eaa1fd

copy-tracking: plumb CopyRecordMap through diff method

ab0b747

copy-tracking: extract next_impl from next in diff iter/stream

b5487d4

copy-tracking: Add copy tracking as a post iteration step

1fc2e6c

- force each diff command to explicitly enable copy tracking - enable copy tracking in diff_summary - post-process for diff iterator - post-process for diff stream - update changelog

fowles force-pushed the simplify branch from 64aec51 to 1fc2e6c Compare August 11, 2024 20:53

fowles enabled auto-merge (rebase) August 11, 2024 20:53

fowles merged commit 5911e5c into jj-vcs:main Aug 11, 2024
17 checks passed

martinvonz mentioned this pull request Aug 12, 2024

copy-tracking: update design doc #4235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

copy-tracking: plumb copy tracking through diff detection #4118

copy-tracking: plumb copy tracking through diff detection #4118

fowles commented Jul 18, 2024 •

edited

Loading

yuja left a comment

fowles commented Jul 19, 2024

martinvonz left a comment

PhilipMetzger left a comment

martinvonz left a comment

martinvonz commented Aug 10, 2024

PhilipMetzger left a comment

copy-tracking: plumb copy tracking through diff detection #4118

copy-tracking: plumb copy tracking through diff detection #4118

Conversation

fowles commented Jul 18, 2024 • edited Loading

yuja left a comment

Choose a reason for hiding this comment

fowles commented Jul 19, 2024

martinvonz left a comment

Choose a reason for hiding this comment

PhilipMetzger left a comment

Choose a reason for hiding this comment

martinvonz left a comment

Choose a reason for hiding this comment

martinvonz commented Aug 10, 2024

PhilipMetzger left a comment

Choose a reason for hiding this comment

fowles commented Jul 18, 2024 •

edited

Loading