Page MenuHomePhabricator

precheck: fix false warning about content-divergence creation
ClosedPublic

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

Details

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

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.

baymax updated this revision to Diff 32716.Mar 26 2022, 1:08 PM

โœ… refresh by Heptapod after a successful CI run (๐Ÿ™ ๐Ÿ’š)
โš  This patch is intended for stable โš 

baymax edited the summary of this revision. (Show Details)Mar 26 2022, 1:56 PM
baymax updated this revision to Diff 32717.

โœ… 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.

av6 added a subscriber: av6.Mar 29 2022, 5:30 AM

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:

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 ?

Alphare accepted this revision.Mar 29 2022, 8:25 AM

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

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
av6 added a comment.Mar 29 2022, 8:45 AM

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.