This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9651

copies-rust: refactor the "deletion" case
ClosedPublic

Authored by marmoute on Dec 21 2020, 5:28 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG2076df13d00f: copies-rust: refactor the "deletion" case
rHGae57e0792c41: copies-rust: refactor the "deletion" case
Summary

We rearrange the code to single out the case where information need to be
overwritten on both side of the merge. This open the way to better dealing with
this case.

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

marmoute created this revision.Dec 21 2020, 5:28 PM
marmoute updated this revision to Diff 24490.Dec 22 2020, 9:12 PM
baymax updated this revision to Diff 24501.Dec 22 2020, 9:46 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 24585.Jan 5 2021, 4:07 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare requested changes to this revision.Jan 8 2021, 8:52 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/copy_tracing.rs
514

This would be better as p1_copies.map(...)

This revision now requires changes to proceed.Jan 8 2021, 8:52 AM
marmoute added inline comments.Jan 8 2021, 9:45 AM
rust/hg-core/src/copy_tracing.rs
514

<suspiciously looking at map(…)>

How would that work ?

Alphare added inline comments.Jan 8 2021, 10:03 AM
rust/hg-core/src/copy_tracing.rs
514
let p1_entry = p1_copies.as_mut().map(
    |copies| match copies.entry(deleted) {
        Entry::Occupied(e) => Some(e),
        Entry::Vacant(_) => None,
    }
);

You need as_mut() as well.

marmoute added inline comments.Jan 8 2021, 10:42 AM
rust/hg-core/src/copy_tracing.rs
514

This would create Option of Option, so I';ll keep the current code.

Alphare accepted this revision.Jan 8 2021, 10:43 AM
Alphare added inline comments.
rust/hg-core/src/copy_tracing.rs
514

Yeah, I meant Option::and, but it seems like the compiler is not able to infer the closure types. So let's keep it that way.

This revision now requires review to proceed.Jan 8 2021, 10:43 AM
baymax updated this revision to Diff 24676.Jan 8 2021, 6:16 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25696.Feb 22 2021, 9:14 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25755.Feb 22 2021, 11:21 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 25794.Feb 22 2021, 5:12 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute added a commit: rHGae57e0792c41: copies-rust: refactor the "deletion" case.Feb 24 2021, 11:02 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGae57e0792c41: copies-rust: refactor the "deletion" case (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/copy_tracing.rs (41 lines)

Diff 25855

rust/hg-core/src/copy_tracing.rs

} }
Action::Removed(deleted_path) => { Action::Removed(deleted_path) => {
// We must drop copy information for removed file. // We must drop copy information for removed file.
// //
// We need to explicitly record them as dropped to // We need to explicitly record them as dropped to
// propagate this information when merging two // propagate this information when merging two
// InternalPathCopies object. // InternalPathCopies object.
let deleted = path_map.tokenize(deleted_path); let deleted = path_map.tokenize(deleted_path);
match &mut p1_copies {
None => (), let p1_entry = match &mut p1_copies {
AlphareUnsubmitted
Not Done

This would be better as p1_copies.map(...)

Alphare: This would be better as `p1_copies.map(...)`
marmouteAuthorUnsubmitted
Done

<suspiciously looking at map(…)>

How would that work ?

marmoute: <suspiciously looking at `map(…)`> How would that work ?
AlphareUnsubmitted
Not Done
let p1_entry = p1_copies.as_mut().map(
    |copies| match copies.entry(deleted) {
        Entry::Occupied(e) => Some(e),
        Entry::Vacant(_) => None,
    }
);

You need as_mut() as well.

Alphare: ``` let p1_entry = p1_copies.as_mut().map( |copies| match copies.entry(deleted) {…
marmouteAuthorUnsubmitted
Done

This would create Option of Option, so I';ll keep the current code.

marmoute: This would create Option of Option, so I';ll keep the current code.
AlphareUnsubmitted
Not Done

Yeah, I meant Option::and, but it seems like the compiler is not able to infer the closure types. So let's keep it that way.

Alphare: Yeah, I meant `Option::and`, but it seems like the compiler is not able to infer the closure…
Some(copies) => { None => None,
copies.entry(deleted).and_modify(|old| { Some(copies) => match copies.entry(deleted) {
old.mark_delete(current_rev); Entry::Occupied(e) => Some(e),
}); Entry::Vacant(_) => None,
},
};
let p2_entry = match &mut p2_copies {
None => None,
Some(copies) => match copies.entry(deleted) {
Entry::Occupied(e) => Some(e),
Entry::Vacant(_) => None,
},
};
match (p1_entry, p2_entry) {
(None, None) => (),
(Some(mut e), None) => {
e.get_mut().mark_delete(current_rev)
}
(None, Some(mut e)) => {
e.get_mut().mark_delete(current_rev)
}
(Some(mut e1), Some(mut e2)) => {
e1.get_mut().mark_delete(current_rev);
e2.get_mut().mark_delete(current_rev);
} }
};
match &mut p2_copies {
None => (),
Some(copies) => {
copies.entry(deleted).and_modify(|old| {
old.mark_delete(current_rev);
});
} }
};
} }
} }
} }
(p1_copies, p2_copies) (p1_copies, p2_copies)
} }
// insert one new copy information in an InternalPathCopies // insert one new copy information in an InternalPathCopies
// //