( )⚙ D11232 rewriteutil: fix crash when a rewritten message references f{6,64}

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11232

rewriteutil: fix crash when a rewritten message references f{6,64}
ClosedPublic

Authored by durin42 on Jul 29 2021, 4:39 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG48da5c325750: rewriteutil: fix crash when a rewritten message references f{6,64}
Summary

Without this, the rewriteutil logic thinks it's found a reference to the wdir
pseudo-revision, and then tries to look it up and rewrite it. Stop it from
doing that.

Amusingly, I had trouble working with this changeset when I didn't
describe the defect above using a regular expression, because it would
trigger the bug in my installed version of hg.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durin42 created this revision.Jul 29 2021, 4:39 PM
mharbison72 added a subscriber: mharbison72.Jul 29 2021, 11:16 PM
mharbison72 added inline comments.
mercurial/rewriteutil.py
210–216

I can't test because I lack easy access to a filesystem with execbit support, but will 0{6,64} also fail?

Also, I didn't think about this when I swiped this from evolve, but should we be locking the repo before doing anything? Otherwise, the lookup here could succeed, but the unfi[fullnode] below could fail.

durin42 marked an inline comment as done.Jul 30 2021, 11:02 AM
durin42 added inline comments.
mercurial/rewriteutil.py
210–216

I don't think so, because nullid will return a node, which won't have any successors.

I don't think we need to lock the repo here.

durin42 marked an inline comment as done.Jul 30 2021, 2:11 PM
durin42 added a commit: rHG48da5c325750: rewriteutil: fix crash when a rewritten message references f{6,64}.
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG48da5c325750: rewriteutil: fix crash when a rewritten message references f{6,64} (authored by durin42). · Explain Why
This revision was automatically updated to reflect the committed changes.
mharbison72 added inline comments.Jul 30 2021, 2:11 PM
mercurial/rewriteutil.py
210–216

I don't think so, because nullid will return a node, which won't have any successors.

OK, I knew repo[nullid] would return something valid, but I wasn't sure about the changelog stuff inside scmutil.resolvehexnodeidprefix. I tweaked the phab test locally, and it looks like it properly stays at 00..00 though, so it looks like it works.

Revision Contents
Changeset List

PathPackages
Mmercurial/rewriteutil.py (7 lines)
Mtests/test-rebase-inmemory.t (10 lines)

Diff 29765

mercurial/rewriteutil.py

of ``pending[oldnode] = [successor_node1, successor_node2,..]``. of ``pending[oldnode] = [successor_node1, successor_node2,..]``.
""" """
if not pending: if not pending:
pending = {} pending = {}
cache = {} cache = {}
hashes = re.findall(NODE_RE, commitmsg) hashes = re.findall(NODE_RE, commitmsg)
unfi = repo.unfiltered() unfi = repo.unfiltered()
for h in hashes: for h in hashes:
try:
fullnode = scmutil.resolvehexnodeidprefix(unfi, h) fullnode = scmutil.resolvehexnodeidprefix(unfi, h)
except error.WdirUnsupported:
# Someone has an fffff... in a commit message we're
# rewriting. Don't try rewriting that.
continue
if fullnode is None: if fullnode is None:
mharbison72Unsubmitted
Done

I can't test because I lack easy access to a filesystem with execbit support, but will 0{6,64} also fail?

Also, I didn't think about this when I swiped this from evolve, but should we be locking the repo before doing anything? Otherwise, the lookup here could succeed, but the unfi[fullnode] below could fail.

mharbison72: I can't test because I lack easy access to a filesystem with execbit support, but will 0{6,64}…
durin42AuthorUnsubmitted
Done

I don't think so, because nullid will return a node, which won't have any successors.

I don't think we need to lock the repo here.

durin42: I don't think so, because nullid will return a node, which won't have any successors. I don't…
mharbison72Unsubmitted
Not Done

I don't think so, because nullid will return a node, which won't have any successors.

OK, I knew repo[nullid] would return something valid, but I wasn't sure about the changelog stuff inside scmutil.resolvehexnodeidprefix. I tweaked the phab test locally, and it looks like it properly stays at 00..00 though, so it looks like it works.

mharbison72: > I don't think so, because nullid will return a node, which won't have any successors. OK, I…
continue continue
ctx = unfi[fullnode] ctx = unfi[fullnode]
if not ctx.obsolete(): if not ctx.obsolete():
successors = pending.get(fullnode) successors = pending.get(fullnode)
if successors is None: if successors is None:
continue continue
# obsutil.successorssets() returns a list of list of nodes # obsutil.successorssets() returns a list of list of nodes
successors = [successors] successors = [successors]

tests/test-rebase-inmemory.t

Test that update_hash_refs works. Test that update_hash_refs works.
$ hg co 0 $ hg co 0
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ echo update_hash_refs > update_hash_refs $ echo update_hash_refs > update_hash_refs
$ hg add update_hash_refs $ hg add update_hash_refs
$ hg ci -m 'this will change hash' $ hg ci -m 'this will change hash'
created new head created new head
$ echo changed >> update_hash_refs $ echo changed >> update_hash_refs
$ hg ci -m "this starts as the child of `hg log -r . -T'{node|short}'` but not 506e2454484b" $ hg ci -m "this starts as the child of `hg log -r . -T'{node|short}'` but not 506e2454484b. Also, ffffffffffffffff"
$ hg tglog $ hg tglog
@ 5: becd28036887 'this starts as the child of 98789aa60148 but not 506e2454484b' @ 5: a8b42cbbde83 'this starts as the child of 98789aa60148 but not 506e2454484b. Also, ffffffffffffffff'
| |
o 4: 98789aa60148 'this will change hash' o 4: 98789aa60148 'this will change hash'
| |
| o 3: 506e2454484b 'merge' | o 3: 506e2454484b 'merge'
| |\ | |\
+---o 2: 531f80391e4a 'c' +---o 2: 531f80391e4a 'c'
| | | |
| o 1: 6f252845ea45 'a' | o 1: 6f252845ea45 'a'
|/ |/
o 0: d20a80d4def3 'base' o 0: d20a80d4def3 'base'
$ hg rebase -r '.^::' -d 3 $ hg rebase -r '.^::' -d 3
rebasing 4:98789aa60148 "this will change hash" rebasing 4:98789aa60148 "this will change hash"
rebasing 5:becd28036887 tip "this starts as the child of 98789aa60148 but not 506e2454484b" rebasing 5:a8b42cbbde83 tip "this starts as the child of 98789aa60148 but not 506e2454484b. Also, ffffffffffffffff"
saved backup bundle to $TESTTMP/keep_merge/.hg/strip-backup/98789aa60148-72ec40bd-rebase.hg saved backup bundle to $TESTTMP/keep_merge/.hg/strip-backup/98789aa60148-da3f4c2c-rebase.hg
$ hg tglog $ hg tglog
@ 5: a445b8426f4b 'this starts as the child of c16c25696fe7 but not 506e2454484b' @ 5: 0fd2912e6cc1 'this starts as the child of c16c25696fe7 but not 506e2454484b. Also, ffffffffffffffff'
| |
o 4: c16c25696fe7 'this will change hash' o 4: c16c25696fe7 'this will change hash'
| |
o 3: 506e2454484b 'merge' o 3: 506e2454484b 'merge'
|\ |\
| o 2: 531f80391e4a 'c' | o 2: 531f80391e4a 'c'
| | | |
o | 1: 6f252845ea45 'a' o | 1: 6f252845ea45 'a'