This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11488

dirstate: Don’t drop unrelated data in DirstateMap::set_entry
ClosedPublic

Authored by SimonSapin on Sep 22 2021, 4:55 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGf6d0a89fdf80: dirstate: Don’t drop unrelated data in DirstateMap::set_entry
Summary

For example, copy source are handled separately. Removing it goes through
the copy_map_remove method (exposed to Python as .copymap.pop())

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.Sep 22 2021, 4:55 PM
Alphare added a subscriber: Alphare.Sep 23 2021, 11:18 AM

I'm surprised no tests were broken. Should there be a (unit) test?

SimonSapin added a comment.Sep 23 2021, 11:42 AM

This is called by dirstatemap.py:

  • In set_tracked when an entry already existed but wasn’t tracked (for example hg add just after hg rm)
  • In set_possibly_dirty
  • In __setitem__
  • (And in set_clean, but that one also calls copymap().pop(filename) anyway)

So my guess as to why no test was affected is that this isn’t called super often, and most tests don’t also check copy tracing information. It would be good to have a CLI-level integration test that’s affected by this but I don’t know how to make one. When and how are used copy source info stored in the dirstate?

I would not be in favor of adding dirstatemap.py unit tests (at least at the moment) because it’s an internal API that keep changing. As to a low-level unit test for this specific DirstateMap::set_entry Rust method, I feel it could only be trivial like "1. Do X 2. Check X is done" and not helpful at finding actual bugs. What matters here is what semantics are expected or not by callers.

Alphare added a comment.Sep 23 2021, 11:47 AM
In D11488#176162, @SimonSapin wrote:

It would be good to have a CLI-level integration test that’s affected by this but I don’t know how to make one. When and how are used copy source info stored in the dirstate?

When using hg move or hg copy, basically. It stores the copy information in the dirstate, and it gets committed at the next hg commit

I would not be in favor of adding dirstatemap.py unit tests (at least at the moment) because it’s an internal API that keep changing. As to a low-level unit test for this specific DirstateMap::set_entry Rust method, I feel it could only be trivial like "1. Do X 2. Check X is done" and not helpful at finding actual bugs. What matters here is what semantics are expected or not by callers.

Agreed.

SimonSapin added a comment.Sep 23 2021, 11:59 AM

Does that mean that a file can only ever have a copy source in the dirstate if it is in "added" state?

Alphare added a comment.Sep 24 2021, 5:54 AM
In D11488#176168, @SimonSapin wrote:

Does that mean that a file can only ever have a copy source in the dirstate if it is in "added" state?

No, because it can be marked as a copy after the fact with hg mv --after for example.

SimonSapin added a comment.Sep 27 2021, 11:34 AM

I’m struggling coming up with a test case that shows different behavior with this patch. I’ve found how to have copy_source != None for some dirstate nodes, but not what hg command can cause this set_entry method to be called for the same node after that.

Alphare accepted this revision.Sep 28 2021, 3:45 AM

I can't find anything obvious either, let's not waste any more time on this. Thanks!

This revision is now accepted and ready to land.Sep 28 2021, 3:45 AM

Revision Contents
Changeset List

Diff 30415

rust/hg-core/src/dirstate_tree/dirstate_map.rs

self.nodes_with_entry_count = 0; self.nodes_with_entry_count = 0;
self.nodes_with_copy_source_count = 0; self.nodes_with_copy_source_count = 0;
} }
fn set_entry(&mut self, filename: &HgPath, entry: DirstateEntry) { fn set_entry(&mut self, filename: &HgPath, entry: DirstateEntry) {
let node = let node =
self.get_or_insert(&filename).expect("no parse error in v1"); self.get_or_insert(&filename).expect("no parse error in v1");
node.data = NodeData::Entry(entry); node.data = NodeData::Entry(entry);
node.children = ChildNodes::default();
node.copy_source = None;
node.descendants_with_entry_count = 0;
node.tracked_descendants_count = 0;
} }
fn add_file( fn add_file(
&mut self, &mut self,
filename: &HgPath, filename: &HgPath,
entry: DirstateEntry, entry: DirstateEntry,
added: bool, added: bool,
merged: bool, merged: bool,