This is an archive of the discontinued Mercurial Phabricator instance.

Differential D715

repair: preserve phase also when not using generaldelta (issue5678)
ClosedPublic

Authored by martinvonz on Sep 14 2017, 2:55 PM.

Details

Reviewers
indygreg
quark
durham
durin42
Group Reviewers
hg-reviewers
Commits
rHG91f0677dc920: repair: preserve phase also when not using generaldelta (issue5678)
Summary

It seems like we used to pick the oldest possible version of the
changegroup to use for bundles created by the repair module (used
e.g. by "hg strip" and for temporary bundles by "hg rebase"). I tried
to preserve that behavior when I created the changegroup.safeversion()
method in 3b2ac2115464 (changegroup: introduce safeversion(),
2016-01-19).

However, we have recently chagned our minds and decided that these
commands are only used locally and downgrades are unlikely. That
decicion allowed us to start adding obsmarker and phase information to
these bundles. However, as the bug report shows, it means we get
different behavior e.g. when generaldelta is not enabled (because when
it was enabled, it forced us to use bundle2). The commit that actually
caused the reported bug was 8e3021fd1a44 (strip: include phases in
bundle (BC), 2017-06-15).

So, since we now depend on having more information in the bundles,
let's make sure we instead pick the newest possible changegroup
version.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Sep 14 2017, 2:55 PM
indygreg accepted this revision as: indygreg.Sep 14 2017, 3:28 PM
indygreg added a subscriber: indygreg.

This seems reasonable to me. I'm not sure it is appropriate for stable though.

The temporary bundles should definitely use the latest available version. But changing the backup bundles (which are persisted after the operation completes) is a BC change and therefore marginally appropriate for stable. I'd feel better if we limited the behavior change on stable to just the temporary bundles and made backup bundle changes on default.

I could be convinced this is appropriate for stable as written if we intended to store the latest available bundle format in backup bundles all along. This does seem reasonable...

martinvonz updated this revision to Diff 1829.Sep 14 2017, 3:42 PM
quark accepted this revision.Sep 14 2017, 3:52 PM
quark added a subscriber: quark.

I have been thinking about moving repo._phasecache.filterunknown(repo) around, or adding some if conditions to skip that for bundles without phases. But that is less cleaner - ex. we also need to revert 168ba5c4dfcb. I prefer cleaner and simpler logic. So I think this is the more desired fix.

martinvonz added a comment.EditedSep 14 2017, 4:01 PM
In D715#11879, @indygreg wrote:

This seems reasonable to me. I'm not sure it is appropriate for stable though.
The temporary bundles should definitely use the latest available version. But changing the backup bundles (which are persisted after the operation completes) is a BC change and therefore marginally appropriate for stable. I'd feel better if we limited the behavior change on stable to just the temporary bundles and made backup bundle changes on default.
I could be convinced this is appropriate for stable as written if we intended to store the latest available bundle format in backup bundles all along. This does seem reasonable...

As I was trying to argue in the commit message, it seemed to be our intention to use the oldest changegroup format until recently. Then our intention changed and we started assuming that obsmarkers and phase information got stored in the bundle. You can see that we depend on obsmarkers being in the bundle from this test case:

$ cat >> $HGRCPATH << EOF
> [experimental]
> evolution=createmarkers
> [format]
> usegeneraldelta=no
> [extensions]
> strip=
> EOF

$ hg init
$ echo a > a
$ hg ci -Aqm a
$ hg ci --amend -m a2
$ hg strip .
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
saved backup bundle to $TESTTMP/.hg/strip-backup/489bac576828-bef27e14-backup.hg (glob)
$ hg unbundle -q .hg/strip-backup/*
$ hg log -G -T '{node}\n'
o  489bac576828490c0bb8d45eac9e5e172e4ec0a8

Without this patch, it fails like this:

@@ -18,3 +18,5 @@
   $ hg log -G -T '{node}\n'
   o  489bac576828490c0bb8d45eac9e5e172e4ec0a8
   
+  o  cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b
+
martinvonz edited the summary of this revision. (Show Details)Sep 14 2017, 4:20 PM
martinvonz updated this revision to Diff 1833.
indygreg accepted this revision.Sep 14 2017, 7:44 PM
This revision is now accepted and ready to land.Sep 14 2017, 7:44 PM
durham accepted this revision.Sep 14 2017, 8:27 PM
durin42 accepted this revision.Sep 15 2017, 11:31 AM
Closed by commit rHG91f0677dc920: repair: preserve phase also when not using generaldelta (issue5678) (authored by martinvonz). · Explain WhySep 15 2017, 11:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1829

mercurial/changegroup.py

# expensive to convert between the flat manifest and tree manifest on # expensive to convert between the flat manifest and tree manifest on
# the fly. Since tree manifests are hashed differently, all of history # the fly. Since tree manifests are hashed differently, all of history
# would have to be converted. Instead, we simply don't even pretend to # would have to be converted. Instead, we simply don't even pretend to
# support versions 01 and 02. # support versions 01 and 02.
versions.discard('01') versions.discard('01')
versions.discard('02') versions.discard('02')
return versions return versions
def localversion(repo):
# Finds the best version to use for bundles that are meant to be used
# locally, such as those from strip and shelve, and temporary bundles.
return max(supportedoutgoingversions(repo))
def safeversion(repo): def safeversion(repo):
# Finds the smallest version that it's safe to assume clients of the repo # Finds the smallest version that it's safe to assume clients of the repo
# will support. For example, all hg versions that support generaldelta also # will support. For example, all hg versions that support generaldelta also
# support changegroup 02. # support changegroup 02.
versions = supportedoutgoingversions(repo) versions = supportedoutgoingversions(repo)
if 'generaldelta' in repo.requirements: if 'generaldelta' in repo.requirements:
versions.discard('01') versions.discard('01')
assert versions assert versions

mercurial/repair.py

vfs.mkdir(backupdir) vfs.mkdir(backupdir)
# Include a hash of all the nodes in the filename for uniqueness # Include a hash of all the nodes in the filename for uniqueness
allcommits = repo.set('%ln::%ln', bases, heads) allcommits = repo.set('%ln::%ln', bases, heads)
allhashes = sorted(c.hex() for c in allcommits) allhashes = sorted(c.hex() for c in allcommits)
totalhash = hashlib.sha1(''.join(allhashes)).hexdigest() totalhash = hashlib.sha1(''.join(allhashes)).hexdigest()
name = "%s/%s-%s-%s.hg" % (backupdir, short(node), totalhash[:8], suffix) name = "%s/%s-%s-%s.hg" % (backupdir, short(node), totalhash[:8], suffix)
cgversion = changegroup.safeversion(repo) cgversion = changegroup.localversion(repo)
comp = None comp = None
if cgversion != '01': if cgversion != '01':
bundletype = "HG20" bundletype = "HG20"
if compress: if compress:
comp = 'BZ' comp = 'BZ'
elif compress: elif compress:
bundletype = "HG10BZ" bundletype = "HG10BZ"
else: else:

tests/test-rebase-scenario-global.t

| |
| o 1:d2ae draft b | o 1:d2ae draft b
|/ |/
o 0:cb9a public a o 0:cb9a public a
$ hg rebase -s 1 -d 2 $ hg rebase -s 1 -d 2
rebasing 1:d2ae7f538514 "b" rebasing 1:d2ae7f538514 "b"
saved backup bundle to $TESTTMP/issue5678/.hg/strip-backup/d2ae7f538514-2953539b-rebase.hg (glob) saved backup bundle to $TESTTMP/issue5678/.hg/strip-backup/d2ae7f538514-2953539b-rebase.hg (glob)
BROKEN: d36c should remain public
$ hg log -G -T '{rev}:{node|shortest} {phase} {desc}\n' $ hg log -G -T '{rev}:{node|shortest} {phase} {desc}\n'
o 2:c882 draft b o 2:c882 draft b
| |
@ 1:d36c draft c @ 1:d36c public c
| |
o 0:cb9a public a o 0:cb9a public a
$ cd .. $ cd ..
Test for revset Test for revset
We need a bit different graph We need a bit different graph