( )⚙ D10083 tags: validate nodes in _getfnodes() and update cache in case of unknown nodes

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10083

tags: validate nodes in _getfnodes() and update cache in case of unknown nodes
ClosedPublic

Authored by pulkit on Mar 1 2021, 1:47 PM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG9a31f65381ae: tags: validate nodes in _getfnodes() and update cache in case of unknown nodes
Summary

hgtagsfnodescache can contain unknown nodes due to cache corruption and this
lead to a traceback on operations like hg tags as we don't validate nodes.

This patch validates that all filenodes returned after hgtagsfnodescache are
known to the repository. If there exists any unknown filenode, we force
recompute it and update the cache.

The test change demonstrates the fix.

Diff Detail

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

Event Timeline

pulkit created this revision.Mar 1 2021, 1:47 PM
marmoute requested changes to this revision.Mar 1 2021, 2:05 PM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
mercurial/tags.py
497–498

lets drop a _ between words here, if you don't mind. This is easier to read.

843–852

We do more that recomputing the nodes, we also update the cache right ? We should maybe update the function name (maybe refresh_invalide_node ?) and update the doc to point this out.

This revision now requires changes to proceed.Mar 1 2021, 2:05 PM
baymax updated this revision to Diff 26013.Mar 2 2021, 3:42 AM

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

pulkit marked 2 inline comments as done.Mar 2 2021, 3:47 AM
marmoute added inline comments.Mar 2 2021, 3:56 AM
mercurial/tags.py
505–506

look slike my main an long comment about this got lost…

I don't understand why we do a full filectx initialisation and data fetch here. What we want to do is only check if the node exist within the filelog. right? Why won't we explicitly grab de filelog and check that?

baymax updated this revision to Diff 26015.Mar 2 2021, 6:44 AM

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

pulkit marked an inline comment as done.Mar 2 2021, 7:34 AM
marmoute requested changes to this revision.Mar 2 2021, 8:28 AM
marmoute added inline comments.
mercurial/tags.py
501

The official way to get this is repo.file(b'.hgtags') if I remember correctly. The current code is a bit abstraction-breaking.

This revision now requires changes to proceed.Mar 2 2021, 8:28 AM
pulkit marked an inline comment as done.Mar 2 2021, 9:24 AM
pulkit updated this revision to Diff 26021.Mar 2 2021, 9:24 AM
marmoute accepted this revision.Mar 5 2021, 12:42 PM
baymax updated this revision to Diff 26106.Mar 5 2021, 1:16 PM

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

pulkit added a commit: rHG9a31f65381ae: tags: validate nodes in _getfnodes() and update cache in case of unknown nodes.Mar 8 2021, 3:26 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG9a31f65381ae: tags: validate nodes in _getfnodes() and update cache in case of unknown nodes (authored by pulkit). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/tags.py (29 lines)
Mtests/test-tags.t (4 lines)

Diff 26021

mercurial/tags.py

"""return .hgtags fnodes for a list of changeset nodes """return .hgtags fnodes for a list of changeset nodes
Return value is a {node: fnode} mapping. There will be no entry for nodes Return value is a {node: fnode} mapping. There will be no entry for nodes
without a '.hgtags' file. without a '.hgtags' file.
""" """
starttime = util.timer() starttime = util.timer()
fnodescache = hgtagsfnodescache(repo.unfiltered()) fnodescache = hgtagsfnodescache(repo.unfiltered())
cachefnode = {} cachefnode = {}
validated_fnodes = set()
unknown_entries = set()
marmouteUnsubmitted
Done

lets drop a _ between words here, if you don't mind. This is easier to read.

marmoute: lets drop a `_` between words here, if you don't mind. This is easier to read.
for node in nodes: for node in nodes:
fnode = fnodescache.getfnode(node) fnode = fnodescache.getfnode(node)
flog = repo.file(b'.hgtags')
marmouteUnsubmitted
Done

The official way to get this is repo.file(b'.hgtags') if I remember correctly. The current code is a bit abstraction-breaking.

marmoute: The official way to get this is `repo.file(b'.hgtags')` if I remember correctly. The current…
if fnode != nullid:
if fnode not in validated_fnodes:
if flog.hasnode(fnode):
validated_fnodes.add(fnode)
else:
marmouteUnsubmitted
Done

look slike my main an long comment about this got lost…

I don't understand why we do a full filectx initialisation and data fetch here. What we want to do is only check if the node exist within the filelog. right? Why won't we explicitly grab de filelog and check that?

marmoute: look slike my main an long comment about this got lost… I don't understand why we do a full…
unknown_entries.add(node)
cachefnode[node] = fnode
if unknown_entries:
fixed_nodemap = fnodescache.refresh_invalid_nodes(unknown_entries)
for node, fnode in pycompat.iteritems(fixed_nodemap):
if fnode != nullid: if fnode != nullid:
cachefnode[node] = fnode cachefnode[node] = fnode
fnodescache.write() fnodescache.write()
duration = util.timer() - starttime duration = util.timer() - starttime
ui.log( ui.log(
b'tagscache', b'tagscache',
b'%d/%d cache hits/lookups in %0.4f seconds\n', b'%d/%d cache hits/lookups in %0.4f seconds\n',
fnodescache.hitcount, fnodescache.hitcount,
ctx = self._repo[node] ctx = self._repo[node]
# Do a lookup first to avoid writing if nothing has changed. # Do a lookup first to avoid writing if nothing has changed.
if self.getfnode(ctx.node(), computemissing=False) == fnode: if self.getfnode(ctx.node(), computemissing=False) == fnode:
return return
self._writeentry(ctx.rev() * _fnodesrecsize, node[0:4], fnode) self._writeentry(ctx.rev() * _fnodesrecsize, node[0:4], fnode)
def refresh_invalid_nodes(self, nodes):
"""recomputes file nodes for a given set of nodes which has unknown
filenodes for them in the cache
Also updates the in-memory cache with the correct filenode.
Caller needs to take care about calling `.write()` so that updates are
persisted.
Returns a map {node: recomputed fnode}
"""
fixed_nodemap = {}
for node in nodes:
marmouteUnsubmitted
Done

We do more that recomputing the nodes, we also update the cache right ? We should maybe update the function name (maybe refresh_invalide_node ?) and update the doc to point this out.

marmoute: We do more that recomputing the nodes, we also update the cache right ? We should maybe update…
fnode = self._computefnode(node)
fixed_nodemap[node] = fnode
self.setfnode(node, fnode)
return fixed_nodemap
def _writeentry(self, offset, prefix, fnode): def _writeentry(self, offset, prefix, fnode):
# Slices on array instances only accept other array. # Slices on array instances only accept other array.
entry = bytearray(prefix + fnode) entry = bytearray(prefix + fnode)
self._raw[offset : offset + _fnodesrecsize] = entry self._raw[offset : offset + _fnodesrecsize] = entry
# self._dirtyoffset could be None. # self._dirtyoffset could be None.
self._dirtyoffset = min(self._dirtyoffset or 0, offset or 0) self._dirtyoffset = min(self._dirtyoffset or 0, offset or 0)
def write(self): def write(self):

tests/test-tags.t

5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d (unknown node) 5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d (unknown node)
$ rm -f .hg/cache/tags2-visible $ rm -f .hg/cache/tags2-visible
$ hg debugtagscache | tail -2 $ hg debugtagscache | tail -2
4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d 4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d
5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d (unknown node) 5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d (unknown node)
$ hg tags $ hg tags
abort: data/.hgtags.i@0c04f2a8deadde17fab7422878ee5a2dadbc943d: no match found tip 5:8dbfe60eff30
[50] bar 1:78391a272241
BUG: Unless this file is restored, the `hg tags` in the next unix-permissions BUG: Unless this file is restored, the `hg tags` in the next unix-permissions
conditional will fail: "abort: data/.hgtags.i@0c04f2a8dead: no match found" conditional will fail: "abort: data/.hgtags.i@0c04f2a8dead: no match found"
$ mv .hg/cache/hgtagsfnodes1.bak .hg/cache/hgtagsfnodes1 $ mv .hg/cache/hgtagsfnodes1.bak .hg/cache/hgtagsfnodes1
#if unix-permissions no-root #if unix-permissions no-root
Errors writing to .hgtags fnodes cache are silently ignored Errors writing to .hgtags fnodes cache are silently ignored