This is an archive of the discontinued Mercurial Phabricator instance.

rewriteutil: look up common predecessor on unfiltered repo
ClosedPublic

Authored by martinvonz on Jun 29 2021, 5:20 PM.

Download Raw Diff

Details

Reviewers

marmoute
Alphare
pulkit

Group Reviewers

hg-reviewers

Commits

rHGdebc29900b97: rewriteutil: look up common predecessor on unfiltered repo
rHG69b35ea9d5f9: rewriteutil: look up common predecessor on unfiltered repo

Summary

Before this patch, the code looking for divergence could crash when
tried to look up a common predecessor in the filtered repo. This patch
fixes that by looking up the common predecessor in an unfiltered repo.

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

martinvonz created this revision.Jun 29 2021, 5:20 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJun 29 2021, 5:20 PM

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

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

This revision now requires changes to proceed.Jun 30 2021, 9:56 AM

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

In D10917#166939, @martinvonz wrote:

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

In D10917#166940, @marmoute wrote:

In D10917#166939, @martinvonz wrote:

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

I still don't understand. Do you mean that find_new_divergence_from() should not return filtered nodeids?

In D10917#166941, @martinvonz wrote:

In D10917#166940, @marmoute wrote:

In D10917#166939, @martinvonz wrote:

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

I still don't understand. Do you mean that find_new_divergence_from() should not return filtered nodeids?

No, given the information returned by find_new_divergence_from() it seems unavoidable that filtered revision might be returned as the "common-predecessor" value. (sidenote: that the common-precessors should probably be a list, and the return of find_new_divergence_from and _find_new_divergence should probably be list too).

My point is that we are looking for divergence within the current filter and that we should not pass unfiltered repository to the function that looks for that divergence. Otherwise it might compute things on the wrong set of revisions.

However, function along the chain should be prepared for the "common-predecessor" nodes to be potentially filtered. In this case this means turning this line

return (repo[r], repo[div[0]], repo[div[1]])

return (repo[r], repo[div[0]], unfi[div[1]])

(sidenode: the previous if div is actually checking if div is not None, so we should update it too)

martinvonz retitled this revision from rewriteutil: check for divergence on unfiltered repo to rewriteutil: look up common predecessor on unfiltered repo.Jul 1 2021, 1:05 PM

martinvonz edited the summary of this revision. (Show Details)

martinvonz updated this revision to Diff 28720.

In D10917#166942, @marmoute wrote:
In D10917#166941, @martinvonz wrote:

In D10917#166940, @marmoute wrote:

In D10917#166939, @martinvonz wrote:

In D10917#166937, @marmoute wrote:

It does not looks like this is done at the right level. The code responsible for the processors walking should be the one adjusting the filter level.

Which code is that? Do you mean that it should be done at a higher or lower level?

lower level

I still don't understand. Do you mean that find_new_divergence_from() should not return filtered nodeids?

No, given the information returned by find_new_divergence_from() it seems unavoidable that filtered revision might be returned as the "common-predecessor" value. (sidenote: that the common-precessors should probably be a list, and the return of find_new_divergence_from and _find_new_divergence should probably be list too).
My point is that we are looking for divergence within the current filter and that we should not pass unfiltered repository to the function that looks for that divergence. Otherwise it might compute things on the wrong set of revisions.
However, function along the chain should be prepared for the "common-predecessor" nodes to be potentially filtered. In this case this means turning this line
return (repo[r], repo[div[0]], repo[div[1]])
to
return (repo[r], repo[div[0]], unfi[div[1]])

I see. I've updated the patch.

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.

Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

(sorry for the delay, I kind of forgot about that patch)

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

I see, that sounds reasonable!

This revision is now accepted and ready to land.Jul 7 2021, 12:07 PM

In D10917#168345, @martinvonz wrote:

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

In D10917#168351, @marmoute wrote:

In D10917#168345, @martinvonz wrote:

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

Good point. I'll try to write a(nother) test case that results in a hidden common predecessor.

In D10917#168352, @martinvonz wrote:

In D10917#168351, @marmoute wrote:

In D10917#168345, @martinvonz wrote:

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

Good point. I'll try to write a(nother) test case that results in a hidden common predecessor.

@Alphare, looks like this patch is not queued yet (which is fine). Do you want me to update the series to have a new test case in the first patch instead? That makes more sense (I can still add the current test case specifically for issue 6262), but maybe you've already queued it locally and it'll be more work for you...

In D10917#168359, @martinvonz wrote:

In D10917#168352, @martinvonz wrote:

In D10917#168351, @marmoute wrote:

In D10917#168345, @martinvonz wrote:

In D10917#168344, @marmoute wrote:

In D10917#168324, @Alphare wrote:

This looks to me like we're moving from one bad output to a different bad output that is arguably more confusing. Maybe I'm not seeing the improvement or there is something else baking behind this?

This is wrongly reported divergence, which is bad, but not the point of this patch.
Before this patch we were crashing while (wrongly) reporting that divergence, now we no longer crash while reporting the divergence. The fact the report is wrong is orthogonal, we should not crash while reporting divergence.

Put another way (explaining to Alphare): Even though this patch doesn't immediately help the user, it helps us hg developers by not forcing us to remember to fix this once we've fixed issue 6262.

Since issue6262 will eventually be fixed, this won't be tested anymore. Would you follow up with a test covering this message + case ?

Good point. I'll try to write a(nother) test case that results in a hidden common predecessor.

@Alphare, looks like this patch is not queued yet (which is fine). Do you want me to update the series to have a new test case in the first patch instead? That makes more sense (I can still add the current test case specifically for issue 6262), but maybe you've already queued it locally and it'll be more work for you...

Go ahead, I hadn't queue it yet. You were saved by the making of food!

martinvonz edited parent revisions, added: D11009: tests: demonstrate crash when common predecessor of divergence is hidden; removed: D10916: tests: add test case for issue 6262.Jul 7 2021, 2:22 PM

martinvonz added a child revision: D10916: tests: add test case for issue 6262.

martinvonz updated this revision to Diff 28945.

pulkit accepted this revision.Jul 8 2021, 4:06 AM

martinvonz added a commit: rHG69b35ea9d5f9: rewriteutil: look up common predecessor on unfiltered repo.Jul 8 2021, 5:02 AM

Closed by commit rHG69b35ea9d5f9: rewriteutil: look up common predecessor on unfiltered repo (authored by martinvonz). · Explain Why

This revision was automatically updated to reflect the committed changes.

martinvonz added a commit: rHGdebc29900b97: rewriteutil: look up common predecessor on unfiltered repo.Jul 8 2021, 7:53 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/rewriteutil.py (2 lines)
M			tests/test-amend.t (6 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	28705		Jun 29 2021, 5:20 PM	★	★
Diff 2	28720		Jul 1 2021, 1:05 PM	★	★
Diff 3	28945		Jul 7 2021, 2:22 PM	★	★
Diff 4	28978	rHG69b35ea9d5f91f547957c889271faab583325d0e	Jun 29 2021, 5:02 PM	★	★

Status	Author	Revision
Closed	martinvonz	D10916 tests: add test case for issue 6262
Closed	martinvonz	D10917 rewriteutil: look up common predecessor on unfiltered repo
Closed	martinvonz	D11009 tests: demonstrate crash when common predecessor of divergence is hidden

Diff 28978

mercurial/rewriteutil.py

	return repo.revs(b"(%ld::) - %ld", revs, revs)			return repo.revs(b"(%ld::) - %ld", revs, revs)


	def _find_new_divergence(repo, revs):			def _find_new_divergence(repo, revs):
	obsrevs = repo.revs(b'%ld and obsolete()', revs)			obsrevs = repo.revs(b'%ld and obsolete()', revs)
	for r in obsrevs:			for r in obsrevs:
	div = find_new_divergence_from(repo, repo[r])			div = find_new_divergence_from(repo, repo[r])
	if div:			if div:
	return (repo[r], repo[div[0]], repo[div[1]])			return (repo[r], repo[div[0]], repo.unfiltered()[div[1]])
	return None			return None


	def find_new_divergence_from(repo, ctx):			def find_new_divergence_from(repo, ctx):
	"""return divergent revision if rewriting an obsolete cset (ctx) will			"""return divergent revision if rewriting an obsolete cset (ctx) will
	create divergence			create divergence

	Returns (<other node>, <common ancestor node>) or None			Returns (<other node>, <common ancestor node>) or None

tests/test-amend.t

	$ hg amend -m C2			$ hg amend -m C2
	1 new orphan changesets			1 new orphan changesets
	Make the common predecessor (C) pruned			Make the common predecessor (C) pruned
	$ hg tag --local --remove C			$ hg tag --local --remove C
	$ hg co C1			$ hg co C1
	0 files updated, 0 files merged, 0 files removed, 0 files unresolved			0 files updated, 0 files merged, 0 files removed, 0 files unresolved
	Try to cause divergence			Try to cause divergence
	$ hg amend -m C11			$ hg amend -m C11
	abort: filtered revision '26805aba1e600a82e93661149f2313866a221a7b' (known-bad-output !)			abort: cannot amend 2758767f5d17, as that creates content-divergence with bfcb433a0dea, from 26805aba1e60
	[255]			(add --verbose for details or see 'hg help evolution.instability')
	[10]			[10]
	#endif			#endif

	Cannot amend public changeset			Cannot amend public changeset

	$ hg phase -r A --public			$ hg phase -r A --public
	$ hg update -C -q A			$ hg update -C -q A
	$ hg amend -m AMEND			$ hg amend -m AMEND
	abort: cannot amend public changesets: 426bada5c675			abort: cannot amend public changesets: 426bada5c675