This is an archive of the discontinued Mercurial Phabricator instance.

fix: add a -s option to format a revision and its descendants
ClosedPublic

Authored by martinvonz on Mar 13 2020, 3:33 PM.

Details

Summary

hg fix -r abc123 will format that commit but not its
descendants. That seems expected given the option name (-r), but
it's very rarely what the user wants to do. The problem is that any
descendants of that commit will not be formatted, leaving them as
orphans that are hard to evolve. They are hard to evolve because the
new parent will have formatting changes that the orphan doesn't have.

I talked to Danny Hooper (who wrote most of the fix extension) about
the problem and we agreed that deprecating -r in favor of a new -s
argument (mimicing rebase's -s) would be a good way of reducing the
risk that users end up with these hard-to-evolve orphans. So that's
what this patch implements.

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

martinvonz created this revision.Mar 13 2020, 3:33 PM
martinvonz updated this revision to Diff 20776.Mar 13 2020, 3:38 PM
hooper added a subscriber: hooper.Mar 13 2020, 5:02 PM

D8283:D8288 all looks good to me.

Thanks for this!

hgext/fix.py
221

nit: is descendants usually self referential? (The revset is, but IDK if that's what a casual reader would expect here.) The same option for rebase says:

rebase the specified changeset and descendants
412–413

Any idea why rebase seems to convert the source arg to a list before passing to revrange(), and also does repo.revs(b'(%ld)::', src) or src? IDK if it's trying to handle something that should be handled here for consistent behavior.

https://www.mercurial-scm.org/repo/hg/file/tip/hgext/rebase.py#l1264

martinvonz added inline comments.Mar 13 2020, 7:23 PM
hgext/fix.py
221

Ah, good point. I'll copy the text from rebase. Thanks!

412–413

Any idea why rebase seems to convert the source arg to a list before passing to revrange(),

I'm pretty sure that's because rebase's --source argument is not list-valued, i.e. hg rebase --source foo --source bar ... will rebase only bar:: (I didn't test that, though). We should probably change that, even though it would be a small BC (same thing with --base).

and also does repo.revs(b'(%ld)::', src) or src?

Oh, maybe that or src is to handle the wdir case I mention two lines down here? Let me check... Yep, that's it: D8057 (written by me just ~6 weeks ago and I had already forgotten :P). We can't replicate that trick here because we support multiple source revisions. We should instead remember to handle that correctly if we teach rebase to support multiple source revisions.

IDK if it's trying to handle something that should be handled here for consistent behavior.

Good point and thanks for checking!

martinvonz updated this revision to Diff 20780.Mar 13 2020, 7:24 PM
mharbison72 accepted this revision.Mar 13 2020, 7:31 PM
martinvonz updated this revision to Diff 20822.Mar 18 2020, 2:34 AM
pulkit accepted this revision.Mar 19 2020, 4:14 AM
This revision is now accepted and ready to land.Mar 19 2020, 4:14 AM