[PATCH 2 of 2] scmutil: add a cleanupnodes method for developers

Jun Wu quark at fb.com
Wed Jun 28 14:08:41 EDT 2017


Excerpts from Martin von Zweigbergk's message of 2017-06-28 10:18:22 -0700:
> I wonder if we should pass in the transaction instead to force the
> caller to think about the transaction scope, even if they all just
> start a new transaction for the cleanup.

Good find. Durham also suggested to remove "operation". I have planned to
add the "desc" information to transaction and use that as operation here.
This function will then require an active transaction, which looks good.

> If we're pruning a merge, this puts the bookmark on an "arbitrary"
> parent. I see that that's how it already works and I suppose you just
> preserved that behavior. Should we simply do the same for the
> divergence case above?  The ProgrammingError is a little unfortunate,
> because the only way to prevent it is by not calling this method, AFAICT
> (sure, you can handle that specific bookmark outside, but then you're not
> saving that much). So maybe just use max() for the divergence too?

The current ProgrammingError only happens when split a commit to something
with multiple roots. Like split A to B and C, and ancestor(B+C) is not in
[B, C]. I think that's a wrong split implementation and therefore raised
ProgrammingError.  Normally you won't be able to get into that situation.

If we don't care about that corner case, I think "max()" makes sense to
simplify the code and user experience.

> Nit: We could probably make that code a bit clearer so the comment
> above about "if s or not isobs(n)" is not needed. Maybe something like
> this:
> 
>             rels = []
>             for node, successors in sorted(mapping.items(), key=sortfunc):
>                 ctx = unfi[node]
>                 if not successors and ctx.obsolete():
>                     continue # already done
>                 rels.append((ctx, (unfi[s] for s in successors)))

Sure. I was just wanting to make the corner case clear: amend A to B, then
prune A. The latter does provide some extra information - intention to
remove A and is in theory a "divergent" from the intention to "amend" A.
That "divergent" is causing unwanted trouble for users since a user will
almost always choose to keep B instead of remove both.

I'll clean up the code and put the explanation in commit message.

> Same comment as on another patch: we're on Python 2.7, so can combine
> these with-blocks.

I actually didn't know that syntax until yesterday :)


More information about the Mercurial-devel mailing list