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

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

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

Commit	Parents	Author	Summary	Date
a358c1304b02	016b36ccd661	Martin von Zweigbergk		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 28705

mercurial/rewriteutil.py

	hint = _(b"see 'hg help evolution.instability'")			hint = _(b"see 'hg help evolution.instability'")
	raise error.InputError(			raise error.InputError(
	_(b"cannot %s changeset, as that will orphan %d descendants")			_(b"cannot %s changeset, as that will orphan %d descendants")
	% (action, len(newunstable)),			% (action, len(newunstable)),
	hint=hint,			hint=hint,
	)			)

	if not obsolete.isenabled(repo, obsolete.allowdivergenceopt):			if not obsolete.isenabled(repo, obsolete.allowdivergenceopt):
	new_divergence = _find_new_divergence(repo, revs)			new_divergence = _find_new_divergence(repo.unfiltered(), revs)
	if new_divergence:			if new_divergence:
	local_ctx, other_ctx, base_ctx = new_divergence			local_ctx, other_ctx, base_ctx = new_divergence
	msg = _(			msg = _(
	b'cannot %s %s, as that creates content-divergence with %s'			b'cannot %s %s, as that creates content-divergence with %s'
	) % (			) % (
	action,			action,
	local_ctx,			local_ctx,
	other_ctx,			other_ctx,

tests/test-amend.t

	$ hg debugobsolete $node_B $node_B_split1 $node_B_split2			$ hg debugobsolete $node_B $node_B_split1 $node_B_split2
	1 new obsolescence markers			1 new obsolescence markers
	obsoleted 1 changesets			obsoleted 1 changesets
	$ hg debugobsolete $node_B_split2			$ hg debugobsolete $node_B_split2
	1 new obsolescence markers			1 new obsolescence markers
	obsoleted 1 changesets			obsoleted 1 changesets
	$ hg co --hidden $node_B_split2			$ hg co --hidden $node_B_split2
	0 files updated, 0 files merged, 0 files removed, 0 files unresolved			0 files updated, 0 files merged, 0 files removed, 0 files unresolved
	BROKEN: Should not complain about a filtered predecessor			BROKEN: Should not be considered divergence (issue6262)
	$ hg amend -m 'revived B-split2'			$ hg amend -m 'revived B-split2'
	abort: filtered revision '16084da537dd8f84cfdb3055c633772269d62e1b' (known-bad-output !)			abort: cannot amend 809fe227532f, as that creates content-divergence with c68306a86921, from 16084da537dd (known-bad-output !)
	[255]			(add --verbose for details or see 'hg help evolution.instability') (known-bad-output !)
				[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