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

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

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–215	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–215	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–215	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)

Commit	Parents	Author	Summary	Date
4c9ffaf0eb69	744b297e3f91	Augie Fackler		Jul 29 2021, 4:23 PM

	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 29740

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
				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…
	if fullnode is None:			if fullnode is None:
	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

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	★	★