This is an archive of the discontinued Mercurial Phabricator instance.

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

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



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

Diff Detail

rHG Mercurial
No Linters Available
No Unit Test Coverage

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.

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

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:

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.