[PATCH 2 of 2] scmutil: add a cleanupnodes method for developers
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
> 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