( )⚙ D10017 tags: return set of invalid nodes from _tagsfromfnodes()

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10017

tags: return set of invalid nodes from _tagsfromfnodes()
AbandonedPublic

Authored by pulkit on Feb 17 2021, 2:44 PM.

Details

Reviewers
Alphare
marmoute
Group Reviewers
hg-reviewers
Summary

We pass a list of file nodes to that function but some can be invalid because
they were read from a cache which was corrupted.

This patches catches LookupError which is being raised because of invalid
fnodes, builds a set of such nodes and return it to the callers.
In next patch, we will add logic to update cache in such cases.

Diff Detail

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

Event Timeline

pulkit created this revision.Feb 17 2021, 2:44 PM
Alphare accepted this revision.Feb 23 2021, 4:42 AM
marmoute accepted this revision.Mar 1 2021, 4:00 AM
marmoute requested changes to this revision.Mar 1 2021, 4:04 AM
marmoute added inline comments.
tests/test-tags.t
454–456

The commit message in D10018 seems to imply that this output is incorrect. Can you flag the bad output with known-bad-output and missing-correct-output ?

This revision now requires changes to proceed.Mar 1 2021, 4:04 AM
marmoute added a comment.Mar 1 2021, 5:39 AM

I am looking at this more, and I don't think we are addressing the issue at the right location.

Here you deal with error while trying to process the data pointed by the tags fnodes and you don't have the necessary information to react well when that fnode is bad.

We should treat this error earlier (maybe in _filterfnodes, or right before it?). At that point I think we could validate that all the fnode we have are correct / or corrected before starting to process them. In such context maybe we could even write down the fnode cache before proceeding further (if it make sense transactionwise).

mercurial/tags.py
120–121

I don't think should not ignore error here.

pulkit planned changes to this revision.Mar 1 2021, 1:54 PM

Superseeded by D10083.

pulkit added a comment.Mar 1 2021, 1:56 PM
In D10017#153000, @marmoute wrote:

I am looking at this more, and I don't think we are addressing the issue at the right location.
Here you deal with error while trying to process the data pointed by the tags fnodes and you don't have the necessary information to react well when that fnode is bad.
We should treat this error earlier (maybe in _filterfnodes, or right before it?). At that point I think we could validate that all the fnode we have are correct / or corrected before starting to process them. In such context maybe we could even write down the fnode cache before proceeding further (if it make sense transactionwise).

Sent D10083 which handles this at much earlier stage.

pulkit abandoned this revision.Mar 9 2021, 2:15 AM

Revision Contents
Changeset List

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

Diff 25655

mercurial/tags.py

The list contains entries in the form: (tagname, oldvalue, new value). The list contains entries in the form: (tagname, oldvalue, new value).
None is used to expressed missing value: None is used to expressed missing value:
('foo', None, 'abcd') is a new tag, ('foo', None, 'abcd') is a new tag,
('bar', 'ef01', None) is a deletion, ('bar', 'ef01', None) is a deletion,
('baz', 'abcd', 'ef01') is a tag movement. ('baz', 'abcd', 'ef01') is a tag movement.
""" """
if oldfnodes == newfnodes: if oldfnodes == newfnodes:
return [] return []
oldtags = _tagsfromfnodes(ui, repo, oldfnodes) oldtags = _tagsfromfnodes(ui, repo, oldfnodes)[0]
newtags = _tagsfromfnodes(ui, repo, newfnodes) newtags = _tagsfromfnodes(ui, repo, newfnodes)[0]
marmouteUnsubmitted
Not Done

I don't think should not ignore error here.

marmoute: I don't think should not ignore error here.
# list of (tag, old, new): None means missing # list of (tag, old, new): None means missing
entries = [] entries = []
for tag, (new, __) in newtags.items(): for tag, (new, __) in newtags.items():
new = _nulltonone(new) new = _nulltonone(new)
old, __ = oldtags.pop(tag, (None, None)) old, __ = oldtags.pop(tag, (None, None))
old = _nulltonone(old) old = _nulltonone(old)
if old != new: if old != new:
_updatetags(cachetags, alltags) _updatetags(cachetags, alltags)
return alltags return alltags
for head in reversed(heads): # oldest to newest for head in reversed(heads): # oldest to newest
assert repo.changelog.index.has_node( assert repo.changelog.index.has_node(
head head
), b"tag cache returned bogus head %s" % short(head) ), b"tag cache returned bogus head %s" % short(head)
fnodes = _filterfnodes(tagfnode, reversed(heads)) fnodes = _filterfnodes(tagfnode, reversed(heads))
alltags = _tagsfromfnodes(ui, repo, fnodes) alltags, invalidnodes = _tagsfromfnodes(ui, repo, fnodes)
# and update the cache (if necessary) # and update the cache (if necessary)
if shouldwrite: if shouldwrite:
_writetagcache(ui, repo, valid, alltags) _writetagcache(ui, repo, valid, alltags)
return alltags return alltags
def _filterfnodes(tagfnode, nodes): def _filterfnodes(tagfnode, nodes):
"""return a list of unique fnodes """return a list of unique fnodes
The order of this list matches the order of "nodes". Preserving this order The order of this list matches the order of "nodes". Preserving this order
is important as reading tags in different order provides different is important as reading tags in different order provides different
results.""" results."""
seen = set() # set of fnode seen = set() # set of fnode
fnodes = [] fnodes = []
for no in nodes: # oldest to newest for no in nodes: # oldest to newest
fnode = tagfnode.get(no) fnode = tagfnode.get(no)
if fnode and fnode not in seen: if fnode and fnode not in seen:
seen.add(fnode) seen.add(fnode)
fnodes.append(fnode) fnodes.append(fnode)
return fnodes return fnodes
def _tagsfromfnodes(ui, repo, fnodes): def _tagsfromfnodes(ui, repo, fnodes):
"""return a tagsmap from a list of file-node """return a tuple
(tagsmap from a list of file-node, list of invalid fnodes)
tagsmap: tag name to (node, hist) 2-tuples. tagsmap: tag name to (node, hist) 2-tuples.
The order of the list matters.""" The order of the list matters."""
alltags = {} alltags = {}
invalidfnodes = set()
fctx = None fctx = None
for fnode in fnodes: for fnode in fnodes:
if fctx is None: if fctx is None:
fctx = repo.filectx(b'.hgtags', fileid=fnode) fctx = repo.filectx(b'.hgtags', fileid=fnode)
else: else:
fctx = fctx.filectx(fnode) fctx = fctx.filectx(fnode)
try:
filetags = _readtags(ui, repo, fctx.data().splitlines(), fctx) filetags = _readtags(ui, repo, fctx.data().splitlines(), fctx)
except error.LookupError:
# some fnodes can be invalid because of broken cache
invalidfnodes.add(fnode)
_updatetags(filetags, alltags) _updatetags(filetags, alltags)
return alltags return (alltags, invalidfnodes)
def readlocaltags(ui, repo, alltags, tagtypes): def readlocaltags(ui, repo, alltags, tagtypes):
'''Read local tags in repo. Update alltags and tagtypes.''' '''Read local tags in repo. Update alltags and tagtypes.'''
try: try:
data = repo.vfs.read(b"localtags") data = repo.vfs.read(b"localtags")
except IOError as inst: except IOError as inst:
if inst.errno != errno.ENOENT: if inst.errno != errno.ENOENT:

tests/test-tags.t

4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d 4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d
5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d (invalid node) 5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d (invalid 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 (invalid node) 5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d (invalid node)
$ hg tags $ hg tags
abort: data/.hgtags.i@0c04f2a8deadde17fab7422878ee5a2dadbc943d: no match found tip 5:8dbfe60eff30
[50] bar 1:78391a272241
marmouteUnsubmitted
Not Done

The commit message in D10018 seems to imply that this output is incorrect. Can you flag the bad output with known-bad-output and missing-correct-output ?

marmoute: The commit message in D10018 seems to imply that this output is incorrect. Can you flag the bad…
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