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

Branch

stable

Lint

No Linters Available

Unit

No Unit Test Coverage

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

Commit	Parents	Author	Summary	Date
f10e9c690ce1	ca0c6d8c456b	Sushil khanchi		Jul 23 2021, 4:12 AM

Diff 31658

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', successors=None):
	"""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.

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

				# Adding a prune marker (A, ()) to obsstore can't be diverging
				if action == b'prune' and not successors:
				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…
				return

	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,