This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9199

upgrade: allow sidedata upgrade to modify revision flag
ClosedPublic

Authored by marmoute on Oct 13 2020, 6:40 AM.

Details

Reviewers
indygreg
pulkit
Group Reviewers
hg-reviewers
Commits
rHGedf4fa06df94: upgrade: allow sidedata upgrade to modify revision flag
Summary

In the process, we fix the lack of HAS_COPIES_INFO flag on upgrade, and test the
results.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

marmoute created this revision.Oct 13 2020, 6:40 AM
pulkit added a subscriber: pulkit.Oct 14 2020, 3:29 AM
pulkit added inline comments.
mercurial/metadata.py
929

IIUC, this should be return False, f, {}, sidedataflag.REVIDX_HASCOPIESINFO, 0 instead of return False, f, {}, 0, sidedataflag.REVIDX_HASCOPIESINFO

mercurial/revlog.py
1494

unrelated change

1774

unrelated change

2045

unrelated change

tests/test-copies-chain-merge.t
1

Hm, didn't quite understand why we need a new case here.

marmoute added inline comments.Oct 14 2020, 4:43 AM
mercurial/metadata.py
929

Should it ? We are removing copy tracing sidedata information here. So we need to drop the flag.

tests/test-copies-chain-merge.t
1

(I just wrote a related reply on D9195)

They are three case I want tested (in addition to compatibility)

  1. how the copy tracing behave when it use information from the filelog
  2. how the copy tracing behave using sidedata generated at commit time
  3. how copy tracing behave using sidedata computed after the fact (the code that sets these metadata is completely different)

That last point is important, because the lack of it lead to breakage to got undetected.

marmoute updated this revision to Diff 23216.Oct 14 2020, 7:47 PM
marmoute marked 2 inline comments as done.Oct 14 2020, 7:48 PM
marmoute added inline comments.
mercurial/revlog.py
1774

These one should be fixed now.

pulkit accepted this revision.Oct 15 2020, 2:57 PM
This revision is now accepted and ready to land.Oct 15 2020, 2:57 PM
marmoute marked an inline comment as done.Oct 15 2020, 5:07 PM
marmoute updated this revision to Diff 23231.
marmoute added a commit: rHGedf4fa06df94: upgrade: allow sidedata upgrade to modify revision flag.Oct 16 2020, 3:48 AM
Closed by commit rHGedf4fa06df94: upgrade: allow sidedata upgrade to modify revision flag (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 23187

mercurial/metadata.py

if sidedata is None: if sidedata is None:
# look at the queued result until we find the one we are lookig # look at the queued result until we find the one we are lookig
# for (shelve the other ones) # for (shelve the other ones)
r, data = sidedataq.get() r, data = sidedataq.get()
while r != rev: while r != rev:
staging[r] = data staging[r] = data
r, sidedata = sidedataq.get() r, sidedata = sidedataq.get()
tokens.release() tokens.release()
sidedataq, has_copies_info = data sidedata, has_copies_info = data
return False, (), sidedata new_flag = 0
if has_copies_info:
new_flag = sidedataflag.REVIDX_HASCOPIESINFO
return False, (), sidedata, new_flag, 0
return sidedata_companion return sidedata_companion
def _get_simple_sidedata_adder(srcrepo, destrepo): def _get_simple_sidedata_adder(srcrepo, destrepo):
"""The simple version of the sidedata computation """The simple version of the sidedata computation
It just compute it in the same thread on request""" It just compute it in the same thread on request"""
def sidedatacompanion(revlog, rev): def sidedatacompanion(revlog, rev):
sidedata = {} sidedata, has_copies_info = {}, False
if util.safehasattr(revlog, 'filteredrevs'): # this is a changelog if util.safehasattr(revlog, 'filteredrevs'): # this is a changelog
sidedata, has_copies_info = _getsidedata(srcrepo, rev) sidedata, has_copies_info = _getsidedata(srcrepo, rev)
return False, (), sidedata new_flag = 0
if has_copies_info:
new_flag = sidedataflag.REVIDX_HASCOPIESINFO
return False, (), sidedata, new_flag, 0
return sidedatacompanion return sidedatacompanion
def getsidedataremover(srcrepo, destrepo): def getsidedataremover(srcrepo, destrepo):
def sidedatacompanion(revlog, rev): def sidedatacompanion(revlog, rev):
f = () f = ()
if util.safehasattr(revlog, 'filteredrevs'): # this is a changelog if util.safehasattr(revlog, 'filteredrevs'): # this is a changelog
if revlog.flags(rev) & sidedataflag.REVIDX_SIDEDATA: if revlog.flags(rev) & sidedataflag.REVIDX_SIDEDATA:
f = ( f = (
sidedatamod.SD_P1COPIES, sidedatamod.SD_P1COPIES,
sidedatamod.SD_P2COPIES, sidedatamod.SD_P2COPIES,
sidedatamod.SD_FILESADDED, sidedatamod.SD_FILESADDED,
sidedatamod.SD_FILESREMOVED, sidedatamod.SD_FILESREMOVED,
) )
return False, f, {} return False, f, {}, 0, sidedataflag.REVIDX_HASCOPIESINFO
pulkitUnsubmitted
Not Done

IIUC, this should be return False, f, {}, sidedataflag.REVIDX_HASCOPIESINFO, 0 instead of return False, f, {}, 0, sidedataflag.REVIDX_HASCOPIESINFO

pulkit: IIUC, this should be `return False, f, {}, sidedataflag.REVIDX_HASCOPIESINFO, 0` instead of…
marmouteAuthorUnsubmitted
Done

Should it ? We are removing copy tracing sidedata information here. So we need to drop the flag.

marmoute: Should it ? We are removing copy tracing sidedata information here. So we need to drop the flag.
return sidedatacompanion return sidedatacompanion

mercurial/revlog.py

if maybewdir: if maybewdir:
raise error.WdirUnsupported raise error.WdirUnsupported
return None return None
except TypeError: except TypeError:
pass pass
def lookup(self, id): def lookup(self, id):
"""locate a node based on: """locate a node based on:
- revision number or str(revision number) - revision number or str(revision number)
pulkitUnsubmitted
Not Done

unrelated change

pulkit: unrelated change
- nodeid or subset of hex nodeid - nodeid or subset of hex nodeid
""" """
n = self._match(id) n = self._match(id)
if n is not None: if n is not None:
return n return n
n = self._partialmatch(id) n = self._partialmatch(id)
if n: if n:
return n return n
if base == rev: if base == rev:
return nullrev return nullrev
elif self._generaldelta: elif self._generaldelta:
return base return base
else: else:
return rev - 1 return rev - 1
def issnapshot(self, rev): def issnapshot(self, rev):
"""tells whether rev is a snapshot """tells whether rev is a snapshot"""
pulkitUnsubmitted
Done

unrelated change

pulkit: unrelated change
marmouteAuthorUnsubmitted
Done

These one should be fixed now.

marmoute: These one should be fixed now.
"""
if not self._sparserevlog: if not self._sparserevlog:
return self.deltaparent(rev) == nullrev return self.deltaparent(rev) == nullrev
elif util.safehasattr(self.index, b'issnapshot'): elif util.safehasattr(self.index, b'issnapshot'):
# directly assign the method to cache the testing and access # directly assign the method to cache the testing and access
self.issnapshot = self.index.issnapshot self.issnapshot = self.index.issnapshot
return self.issnapshot(rev) return self.issnapshot(rev)
if rev == nullrev: if rev == nullrev:
return True return True
# the temp file replace the real index when we exit the context # the temp file replace the real index when we exit the context
# manager # manager
tr.replace(self.indexfile, trindex * self._io.size) tr.replace(self.indexfile, trindex * self._io.size)
nodemaputil.setup_persistent_nodemap(tr, self) nodemaputil.setup_persistent_nodemap(tr, self)
self._chunkclear() self._chunkclear()
def _nodeduplicatecallback(self, transaction, node): def _nodeduplicatecallback(self, transaction, node):
"""called when trying to add a node already stored. """called when trying to add a node already stored."""
pulkitUnsubmitted
Done

unrelated change

pulkit: unrelated change
"""
def addrevision( def addrevision(
self, self,
text, text,
transaction, transaction,
link, link,
p1, p1,
p2, p2,
argument controls whether to force compute deltas against both parents argument controls whether to force compute deltas against both parents
for merges. By default, the current default is used. for merges. By default, the current default is used.
If not None, the `sidedatacompanion` is callable that accept two If not None, the `sidedatacompanion` is callable that accept two
arguments: arguments:
(srcrevlog, rev) (srcrevlog, rev)
and return a triplet that control changes to sidedata content from the and return a quintet that control changes to sidedata content from the
old revision to the new clone result: old revision to the new clone result:
(dropall, filterout, update) (dropall, filterout, update, new_flags, dropped_flags)
* if `dropall` is True, all sidedata should be dropped * if `dropall` is True, all sidedata should be dropped
* `filterout` is a set of sidedata keys that should be dropped * `filterout` is a set of sidedata keys that should be dropped
* `update` is a mapping of additionnal/new key -> value * `update` is a mapping of additionnal/new key -> value
* new_flags is a bitfields of new flags that the revision should get
* dropped_flags is a bitfields of new flags that the revision shoudl not longer have
""" """
if deltareuse not in self.DELTAREUSEALL: if deltareuse not in self.DELTAREUSEALL:
raise ValueError( raise ValueError(
_(b'value for deltareuse invalid: %s') % deltareuse _(b'value for deltareuse invalid: %s') % deltareuse
) )
if len(destrevlog): if len(destrevlog):
raise ValueError(_(b'destination revlog is not empty')) raise ValueError(_(b'destination revlog is not empty'))
# Some classes override linkrev to take filtered revs into # Some classes override linkrev to take filtered revs into
# account. Use raw entry from index. # account. Use raw entry from index.
flags = entry[0] & 0xFFFF flags = entry[0] & 0xFFFF
linkrev = entry[4] linkrev = entry[4]
p1 = index[entry[5]][7] p1 = index[entry[5]][7]
p2 = index[entry[6]][7] p2 = index[entry[6]][7]
node = entry[7] node = entry[7]
sidedataactions = (False, [], {}) sidedataactions = (False, [], {}, 0, 0)
if sidedatacompanion is not None: if sidedatacompanion is not None:
sidedataactions = sidedatacompanion(self, rev) sidedataactions = sidedatacompanion(self, rev)
# (Possibly) reuse the delta from the revlog if allowed and # (Possibly) reuse the delta from the revlog if allowed and
# the revlog chunk is a delta. # the revlog chunk is a delta.
cachedelta = None cachedelta = None
rawtext = None rawtext = None
if any(sidedataactions) or deltareuse == self.DELTAREUSEFULLADD: if any(sidedataactions) or deltareuse == self.DELTAREUSEFULLADD:
dropall, filterout, update = sidedataactions dropall = sidedataactions[0]
filterout = sidedataactions[1]
update = sidedataactions[2]
new_flags = sidedataactions[3]
dropped_flags = sidedataactions[4]
text, sidedata = self._revisiondata(rev) text, sidedata = self._revisiondata(rev)
if dropall: if dropall:
sidedata = {} sidedata = {}
for key in filterout: for key in filterout:
sidedata.pop(key, None) sidedata.pop(key, None)
sidedata.update(update) sidedata.update(update)
if not sidedata: if not sidedata:
sidedata = None sidedata = None
flags |= new_flags
flags &= ~dropped_flags
destrevlog.addrevision( destrevlog.addrevision(
text, text,
tr, tr,
linkrev, linkrev,
p1, p1,
p2, p2,
cachedelta=cachedelta, cachedelta=cachedelta,
node=node, node=node,

mercurial/upgrade.py

sidedatacompanion = None sidedatacompanion = None
removedreqs = srcrepo.requirements - dstrepo.requirements removedreqs = srcrepo.requirements - dstrepo.requirements
addedreqs = dstrepo.requirements - srcrepo.requirements addedreqs = dstrepo.requirements - srcrepo.requirements
if requirements.SIDEDATA_REQUIREMENT in removedreqs: if requirements.SIDEDATA_REQUIREMENT in removedreqs:
def sidedatacompanion(rl, rev): def sidedatacompanion(rl, rev):
rl = getattr(rl, '_revlog', rl) rl = getattr(rl, '_revlog', rl)
if rl.flags(rev) & revlog.REVIDX_SIDEDATA: if rl.flags(rev) & revlog.REVIDX_SIDEDATA:
return True, (), {} return True, (), {}, 0, 0
return False, (), {} return False, (), {}, 0, 0
elif requirements.COPIESSDC_REQUIREMENT in addedreqs: elif requirements.COPIESSDC_REQUIREMENT in addedreqs:
sidedatacompanion = metadata.getsidedataadder(srcrepo, dstrepo) sidedatacompanion = metadata.getsidedataadder(srcrepo, dstrepo)
elif requirements.COPIESSDC_REQUIREMENT in removedreqs: elif requirements.COPIESSDC_REQUIREMENT in removedreqs:
sidedatacompanion = metadata.getsidedataremover(srcrepo, dstrepo) sidedatacompanion = metadata.getsidedataremover(srcrepo, dstrepo)
return sidedatacompanion return sidedatacompanion

tests/test-copies-chain-merge.t

#testcases filelog compatibility sidedata #testcases filelog compatibility sidedata upgraded
pulkitUnsubmitted
Not Done

Hm, didn't quite understand why we need a new case here.

pulkit: Hm, didn't quite understand why we need a new case here.
marmouteAuthorUnsubmitted
Done

(I just wrote a related reply on D9195)

They are three case I want tested (in addition to compatibility)

  1. how the copy tracing behave when it use information from the filelog
  2. how the copy tracing behave using sidedata generated at commit time
  3. how copy tracing behave using sidedata computed after the fact (the code that sets these metadata is completely different)

That last point is important, because the lack of it lead to breakage to got undetected.

marmoute: (I just wrote a related reply on D9195) They are three case I want tested (in addition to…
===================================================== =====================================================
Test Copy tracing for chain of copies involving merge Test Copy tracing for chain of copies involving merge
===================================================== =====================================================
This test files covers copies/rename case for a chains of commit where merges This test files covers copies/rename case for a chains of commit where merges
are involved. It cheks we do not have unwanted update of behavior and that the are involved. It cheks we do not have unwanted update of behavior and that the
different options to retrieve copies behave correctly. different options to retrieve copies behave correctly.
Test that sidedata computations during upgrades are correct Test that sidedata computations during upgrades are correct
=========================================================== ===========================================================
We upgrade a repository that is not using sidedata (the filelog case) and We upgrade a repository that is not using sidedata (the filelog case) and
check that the same side data have been generated as if they were computed at check that the same side data have been generated as if they were computed at
commit time. commit time.
#if filelog #if upgraded
$ cat >> $HGRCPATH << EOF $ cat >> $HGRCPATH << EOF
> [format] > [format]
> exp-use-side-data = yes > exp-use-side-data = yes
> exp-use-copies-side-data-changeset = yes > exp-use-copies-side-data-changeset = yes
> EOF > EOF
$ hg debugformat -v $ hg debugformat -v
format-variant repo config default format-variant repo config default
fncache: yes yes yes fncache: yes yes yes
requirements requirements
preserved: * (glob) preserved: * (glob)
added: exp-copies-sidedata-changeset, exp-sidedata-flag added: exp-copies-sidedata-changeset, exp-sidedata-flag
#endif #endif
#if no-compatibility #if no-compatibility no-filelog
$ for rev in `hg log --rev 'all()' -T '{rev}\n'`; do $ for rev in `hg log --rev 'all()' -T '{rev}\n'`; do
> echo "##### revision $rev #####" > echo "##### revision $rev #####"
> hg debugsidedata -c -v -- $rev > hg debugsidedata -c -v -- $rev
> hg debugchangedfiles $rev > hg debugchangedfiles $rev
> done > done
##### revision 0 ##### ##### revision 0 #####
1 sidedata entries 1 sidedata entries
##### revision 33 ##### ##### revision 33 #####
1 sidedata entries 1 sidedata entries
entry-0014 size 14 entry-0014 size 14
'\x00\x00\x00\x01\x10\x00\x00\x00\x01\x00\x00\x00\x00d' '\x00\x00\x00\x01\x10\x00\x00\x00\x01\x00\x00\x00\x00d'
salvaged : d, ; salvaged : d, ;
#endif #endif
Downgrade to keep testing the filelog algorithm
(This can be removed once we have an explicite "upgrade" tests case_
#if filelog
$ cat >> $HGRCPATH << EOF
> [format]
> exp-use-side-data = no
> exp-use-copies-side-data-changeset = no
> EOF
$ hg debugformat -v
format-variant repo config default
fncache: yes yes yes
dotencode: yes yes yes
generaldelta: yes yes yes
sparserevlog: yes yes yes
sidedata: yes no no
persistent-nodemap: no no no
copies-sdc: yes no no
plain-cl-delta: yes yes yes
compression: * (glob)
compression-level: default default default
$ hg debugupgraderepo --run --quiet
upgrade will perform the following actions:
requirements
preserved: * (glob)
removed: exp-copies-sidedata-changeset, exp-sidedata-flag
#endif
Test copy information chaining Test copy information chaining
============================== ==============================
merging with unrelated change does not interfere with the renames merging with unrelated change does not interfere with the renames
--------------------------------------------------------------- ---------------------------------------------------------------
- rename on one side - rename on one side

tests/testlib/ext-sidedata.py

finally: finally:
del revlog.sidedatanocheck del revlog.sidedatanocheck
## let's store some arbitrary data just for testing ## let's store some arbitrary data just for testing
# text length # text length
update[sidedata.SD_TEST1] = struct.pack('>I', len(text)) update[sidedata.SD_TEST1] = struct.pack('>I', len(text))
# and sha2 hashes # and sha2 hashes
sha256 = hashlib.sha256(text).digest() sha256 = hashlib.sha256(text).digest()
update[sidedata.SD_TEST2] = struct.pack('>32s', sha256) update[sidedata.SD_TEST2] = struct.pack('>32s', sha256)
return False, (), update return False, (), update, 0, 0
return sidedatacompanion return sidedatacompanion
def extsetup(ui): def extsetup(ui):
extensions.wrapfunction(revlog.revlog, 'addrevision', wrapaddrevision) extensions.wrapfunction(revlog.revlog, 'addrevision', wrapaddrevision)
extensions.wrapfunction(revlog.revlog, 'revision', wraprevision) extensions.wrapfunction(revlog.revlog, 'revision', wraprevision)
extensions.wrapfunction( extensions.wrapfunction(
upgrade, 'getsidedatacompanion', wrapgetsidedatacompanion upgrade, 'getsidedatacompanion', wrapgetsidedatacompanion
) )