working_copy: calculate, store, compare content hash of materialized conflicts #7399
Conversation
| * `jj` now records content hash of materialized conflict files in addition to | ||
| file modification time, and relies on them to detect changes. Unchanged | ||
| conflicts checked out by old `jj` may be snapshotted by new `jj`. Run `jj new | ||
| <REVISION-WITHOUT-CONFLICTS> && jj undo` to check out fresh conflict files. | ||
|
|
There was a problem hiding this comment.
As a user, I don't think it's clear under what circumstances I need to run those commands and what happens if I don't. It sounds like the purpose of the hash is to handle some corner case (do we have a bug report?), but is that case common enough that we even need to tell users how to work around it? Or did I misunderstand the purpose of the hash?
There was a problem hiding this comment.
The goal is to remove re-materialization when parsing conflicts.
Since existing checkout doesn't have a content hash, touch conflict_file && jj stat may snapshot unchanged conflict as different set of added/removed files. I don't feel strongly whether we should suggest a workaround command (because the situation would be rare), but I think formatting changes should be documented.
There was a problem hiding this comment.
If the file's timestamp has already been updated, will jj new <REVISION-WITHOUT-CONFLICTS> && jj undo still be able to restore it to an unmodified state? Since the jj new there does snapshotting, I would think it would not help. So do you actually need jj new <REVISION-WITHOUT-CONFLICTS> && <upgrade jj> && jj undo? IIUC, the confusing thing is that we might snapshot some file and make it look modified but with an empty diff (just a ... line). jj restore <path> can be used for fixing that, right? Should we suggest that instead?
yuja
force-pushed
the
push-smwzwnwzwpuq
branch
from
6792815 to
01d5bbf
Compare
This test would fail if we removed comparison of materialized contents from conflicts::update_from_content().
I'm going to add content hash field to remove re-materialization of conflicts from update_from_content(). This patch also removes Copy because content hash will be stored as Vec<u8>.
yuja
force-pushed
the
push-smwzwnwzwpuq
branch
from
01d5bbf to
df43517
Compare
This will help detect unchanged conflict files.
…licts It seemed odd that we had to pass ConflictMarkerStyle to snapshot functions. Since we just need to check if the on-disk data has changed, we can instead store and compare content hashes. Conflict files checked out by old jj might be updated on mtime change, which seems okay. It's less likely to leave conflict files in the working copy for long and touch them. updated_from_content() is called by merge tools, and unchanged conflicts may be updated there. I think that's okay since "jj resolve" is an explicit operation. test_materialize_parse_roundtrip_different_markers() is simplified as there should no longer be "pair" of marker styles.
yuja
force-pushed
the
push-smwzwnwzwpuq
branch
from
df43517 to
0938a26
Compare
There was a problem hiding this comment.
Btw, it seems like we should make update_from_content() better at preserving the original merge. If the user modified only the conflict hunk, then it seems like we should not update the other parts of the merge. This is also related to #4152.
| * `jj` now records content hash of materialized conflict files in addition to | ||
| file modification time, and relies on them to detect changes. Unchanged | ||
| conflicts checked out by old `jj` may be snapshotted by new `jj`. Run `jj new | ||
| <REVISION-WITHOUT-CONFLICTS> && jj undo` to check out fresh conflict files. | ||
|
|
There was a problem hiding this comment.
If the file's timestamp has already been updated, will jj new <REVISION-WITHOUT-CONFLICTS> && jj undo still be able to restore it to an unmodified state? Since the jj new there does snapshotting, I would think it would not help. So do you actually need jj new <REVISION-WITHOUT-CONFLICTS> && <upgrade jj> && jj undo? IIUC, the confusing thing is that we might snapshot some file and make it look modified but with an empty diff (just a ... line). jj restore <path> can be used for fixing that, right? Should we suggest that instead?
Might be better to merge after the release.
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)