Page MenuHomePhabricator

upgrade: don't create store backup if `--no-backup` is passed
ClosedPublic

Authored by pulkit on Jan 14 2021, 7:54 AM.

Details

Summary

If the user explicitly mentioned that they don't need backup, then let's not
create it.

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

pulkit created this revision.Jan 14 2021, 7:54 AM
mharbison72 accepted this revision.Jan 18 2021, 9:15 PM
This revision is now accepted and ready to land.Jan 18 2021, 9:15 PM
baymax updated this revision to Diff 25198.Jan 21 2021, 9:28 AM

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

indygreg added inline comments.
mercurial/upgrade_utils/engine.py
418

This change makes the repository inconsistent for longer than before.

With the old code, we performed 2 atomic directory renames, which likely completely ~instantaneously, leaving the repository inconsistent for a short window.

With this new code path, we perform the equivalent of rm -rf on store/ then perform an atomic rename of the new store to the proper path.

The problem with the new code path is that the recursive delete can take seconds to perform, leaving the repository inconsistent for that duration.

A better solution would be to rename the old store directory, swap in the new store (the old code path), then perform the rmtree() after the new store is installed.

pulkit added inline comments.Feb 6 2021, 2:26 PM
mercurial/upgrade_utils/engine.py
418

TBH, I am on edge that whether having store inconsistent for less time is helping us or preventing faster upgrades. Not talking about this specific patch but an attempt at a different thing that received a similar objection: https://phab.mercurial-scm.org/D9677

As far as this patch is concerned, actually, we used the better solution before and I undid it in this patch. Feel free to revert it as I don't feel strongly about it.