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.

Download Raw Diff

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJul 29 2021, 4:39 PM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

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

			Path	Packages
M			mercurial/rewriteutil.py (7 lines)
M			tests/test-rebase-inmemory.t (10 lines)

	Status	Author	Revision
	Closed	durin42	D11232 rewriteutil: fix crash when a rewritten message references f{6,64}
	Closed	durin42	D11231 tests: add explicit coverage for update_hash_refs from rewriteutil

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'

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	29740		Jul 29 2021, 4:39 PM	★	★
Diff 2	29765	rHG48da5c32575063bf8835f83544bd116fe08c33f8	Jul 29 2021, 4:23 PM	★	★