This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12002

precheck: fix false warning about content-divergence creation
ClosedPublic

Authored by khanchi97 on Jan 16 2022, 10:53 AM.

Download Raw Diff

Details

Reviewers

martinvonz
marmoute
Alphare

Group Reviewers

hg-reviewers

Commits

rHGd4752aeb20f1: precheck: fix false warning about content-divergence creation

Summary

Before this patch, if we try to hg prune (without any successors) an
already obsoleted cset which has at least one successor, it would false
warn about new content-divergence. As we know, pruning cset without any
successors can not create any divergence.

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

khanchi97 created this revision.Jan 16 2022, 10:53 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJan 16 2022, 10:53 AM

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

Is there an easy test you can make for this?

martinvonz requested changes to this revision.Jan 25 2022, 12:30 PM

martinvonz added a subscriber: martinvonz.

martinvonz added inline comments.

mercurial/rewriteutil.py
92	Depending on `action` seems ugly. What if I create my own `hg pare` command with similar behavior, then I still need to pass "prune" here and my users would see that in error messages? Also `successors` is used only here... Can we simply pass a `check_divergence=True` instead?

This revision now requires changes to proceed.Jan 25 2022, 12:30 PM

marmoute requested changes to this revision.Feb 2 2022, 11:52 AM

marmoute added a subscriber: marmoute.

marmoute added inline comments.

mercurial/rewriteutil.py
92	I agree with martin that we should not use the `action` here. The actions are purely here to provides some information to the users, and shouldn't be used for logic.

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

I'm assuming that the callers that would use check_divergence=False exist in evolve? This patch does not introduce any such calls.

In D12002#189963, @Alphare wrote:

I'm assuming that the callers that would use check_divergence=False exist in evolve? This patch does not introduce any such calls.

Correct. There's no prune or similar command in core: everything that uses precheck() creates successors for revs (which can potentially diverge). So the only user would be prune command in evolve.

In D12002#189996, @av6 wrote:

In D12002#189963, @Alphare wrote:

I'm assuming that the callers that would use check_divergence=False exist in evolve? This patch does not introduce any such calls.

Correct. There's no prune or similar command in core: everything that uses precheck() creates successors for revs (which can potentially diverge). So the only user would be prune command in evolve.

Doesn't histedit, has drop command ?

If the histedit discussion finds something that needs to be adjusted there, it can be resolved in a separate patch

khanchi97 added a commit: rHGd4752aeb20f1: precheck: fix false warning about content-divergence creation.Tue, Mar 29, 8:29 AM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHGd4752aeb20f1: precheck: fix false warning about content-divergence creation (authored by khanchi97). · Explain Why

This revision was automatically updated to reflect the committed changes.

What happens in histedit is it calls precheck() for all revisions in the stack, with no easy way to see what operations are planned for each changeset. To be able to use check_divergence=False only for some changesets (that are going to be dropped), we could iterate over the stack and call precheck() for every revision separately, somewhere where we know the planned operation. I can't think of anything better right now.

Having check_divergence keyword argument still works for that case, so this patch should be okay to land.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/rewriteutil.py (8 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	31658		Jan 16 2022, 10:53 AM	★	★
Diff 2	32716		Sat, Mar 26, 1:08 PM	★	★
Diff 3	32717		Sat, Mar 26, 1:56 PM	★	★
Diff 4	32726	rHGd4752aeb20f1f827d10ee0b877713de1a4e31c89	Jul 23 2021, 4:12 AM	★	★

Diff 32726

mercurial/rewriteutil.py

	summary = b', '.join(shorts)			summary = b', '.join(shorts)
	else:			else:
	first = revs.first()			first = revs.first()
	summary = _(b'%s and %d others')			summary = _(b'%s and %d others')
	summary %= (node.short(tonode(first)), numrevs - 1)			summary %= (node.short(tonode(first)), numrevs - 1)
	return summary			return summary


	def precheck(repo, revs, action=b'rewrite'):			def precheck(repo, revs, action=b'rewrite', check_divergence=True):
	"""check if revs can be rewritten			"""check if revs can be rewritten
	action is used to control the error message.			action is used to control the error message.

				check_divergence allows skipping the divergence checks in cases like adding
				a prune marker (A, ()) to obsstore (which can't be diverging).

	Make sure this function is called after taking the lock.			Make sure this function is called after taking the lock.
	"""			"""
	if nullrev in revs:			if nullrev in revs:
	msg = _(b"cannot %s the null revision") % action			msg = _(b"cannot %s the null revision") % action
	hint = _(b"no changeset checked out")			hint = _(b"no changeset checked out")
	raise error.InputError(msg, hint=hint)			raise error.InputError(msg, hint=hint)

	if any(util.safehasattr(r, 'rev') for r in revs):			if any(util.safehasattr(r, 'rev') for r in revs):
	if newunstable:			if newunstable:
	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 check_divergence:
				return
				martinvonzUnsubmitted Not Done Depending on `action` seems ugly. What if I create my own `hg pare` command with similar behavior, then I still need to pass "prune" here and my users would see that in error messages? Also `successors` is used only here... Can we simply pass a `check_divergence=True` instead? martinvonz: Depending on `action` seems ugly. What if I create my own `hg pare` command with similar…
				marmouteUnsubmitted Not Done I agree with martin that we should not use the `action` here. The actions are purely here to provides some information to the users, and shouldn't be used for logic. marmoute: I agree with martin that we should not use the `action` here. The actions are purely here to…

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