This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8287

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

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

Details

Reviewers
mharbison72
pulkit
Group Reviewers
hg-reviewers
Commits
rHG5205b46bd887: fix: add a -s option to format a revision and its descendants
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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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.

mharbison72 added a subscriber: mharbison72.Mar 13 2020, 6:41 PM

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

Revision Contents
Changeset List

PathPackages
Mhgext/fix.py (21 lines)
Mtests/test-fix-topology.t (106 lines)
Mtests/test-fix.t (13 lines)

Diff 20780

hgext/fix.py

_( _(
b'revisions to diff against (overrides automatic ' b'revisions to diff against (overrides automatic '
b'selection, and applies to every revision being ' b'selection, and applies to every revision being '
b'fixed)' b'fixed)'
), ),
_(b'REV'), _(b'REV'),
) )
revopt = (b'r', b'rev', [], _(b'revisions to fix'), _(b'REV')) revopt = (b'r', b'rev', [], _(b'revisions to fix'), _(b'REV'))
sourceopt = (
b's',
b'source',
[],
_(b'fix the specified revisions and their descendants'),
mharbison72Unsubmitted
Not Done

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
mharbison72: nit: is `descendants` usually self referential? (The revset is, but IDK if that's what a…
martinvonzAuthorUnsubmitted
Done

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

martinvonz: Ah, good point. I'll copy the text from rebase. Thanks!
_(b'REV'),
)
wdiropt = (b'w', b'working-dir', False, _(b'fix the working directory')) wdiropt = (b'w', b'working-dir', False, _(b'fix the working directory'))
wholeopt = (b'', b'whole', False, _(b'always fix every line of a file')) wholeopt = (b'', b'whole', False, _(b'always fix every line of a file'))
usage = _(b'[OPTION]... [FILE]...') usage = _(b'[OPTION]... [FILE]...')
@command( @command(
b'fix', b'fix',
[allopt, baseopt, revopt, wdiropt, wholeopt], [allopt, baseopt, revopt, sourceopt, wdiropt, wholeopt],
usage, usage,
helpcategory=command.CATEGORY_FILE_CONTENTS, helpcategory=command.CATEGORY_FILE_CONTENTS,
) )
def fix(ui, repo, *pats, **opts): def fix(ui, repo, *pats, **opts):
"""rewrite file content in changesets or working directory """rewrite file content in changesets or working directory
Runs any configured tools to fix the content of files. Only affects files Runs any configured tools to fix the content of files. Only affects files
with changes, unless file arguments are provided. Only affects changed lines with changes, unless file arguments are provided. Only affects changed lines
When determining what lines of each file to fix at each revision, the whole When determining what lines of each file to fix at each revision, the whole
set of revisions being fixed is considered, so that fixes to earlier set of revisions being fixed is considered, so that fixes to earlier
revisions are not forgotten in later ones. The --base flag can be used to revisions are not forgotten in later ones. The --base flag can be used to
override this default behavior, though it is not usually desirable to do so. override this default behavior, though it is not usually desirable to do so.
""" """
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
cmdutil.check_at_most_one_arg(opts, b'all', b'rev') cmdutil.check_at_most_one_arg(opts, b'all', b'rev')
cmdutil.check_incompatible_arguments(opts, b'working_dir', [b'all']) cmdutil.check_incompatible_arguments(
opts, b'working_dir', [b'all', b'source']
)
with repo.wlock(), repo.lock(), repo.transaction(b'fix'): with repo.wlock(), repo.lock(), repo.transaction(b'fix'):
revstofix = getrevstofix(ui, repo, opts) revstofix = getrevstofix(ui, repo, opts)
basectxs = getbasectxs(repo, opts, revstofix) basectxs = getbasectxs(repo, opts, revstofix)
workqueue, numitems = getworkqueue( workqueue, numitems = getworkqueue(
ui, repo, pats, opts, revstofix, basectxs ui, repo, pats, opts, revstofix, basectxs
) )
fixers = getfixers(ui) fixers = getfixers(ui)
numitems[rev] += 1 numitems[rev] += 1
return workqueue, numitems return workqueue, numitems
def getrevstofix(ui, repo, opts): def getrevstofix(ui, repo, opts):
"""Returns the set of revision numbers that should be fixed""" """Returns the set of revision numbers that should be fixed"""
if opts[b'all']: if opts[b'all']:
revs = repo.revs(b'(not public() and not obsolete()) or wdir()') revs = repo.revs(b'(not public() and not obsolete()) or wdir()')
elif opts[b'source']:
source_revs = scmutil.revrange(repo, opts[b'source'])
revs = set(repo.revs(b'%ld::', source_revs))
mharbison72Unsubmitted
Not Done

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

mharbison72: Any idea why rebase seems to convert the `source` arg to a list before passing to `revrange()`…
martinvonzAuthorUnsubmitted
Done

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: > Any idea why rebase seems to convert the source arg to a list before passing to revrange()…
if wdirrev in source_revs:
# `wdir()::` is currently empty, so manually add wdir
revs.add(wdirrev)
if repo[b'.'].rev() in revs:
revs.add(wdirrev)
else: else:
revs = set(scmutil.revrange(repo, opts[b'rev'])) revs = set(scmutil.revrange(repo, opts[b'rev']))
if opts.get(b'working_dir'): if opts.get(b'working_dir'):
revs.add(wdirrev) revs.add(wdirrev)
for rev in revs: for rev in revs:
checkfixablectx(ui, repo, repo[rev]) checkfixablectx(ui, repo, repo[rev])
# Allow fixing only wdir() even if there's an unfinished operation # Allow fixing only wdir() even if there's an unfinished operation
if not (len(revs) == 1 and wdirrev in revs): if not (len(revs) == 1 and wdirrev in revs):

tests/test-fix-topology.t

Tests for the fix extension's behavior around non-trivial history topologies. Tests for the fix extension's behavior around non-trivial history topologies.
Looks for correct incremental fixing and reproduction of parent/child Looks for correct incremental fixing and reproduction of parent/child
relationships. We indicate fixed file content by uppercasing it. relationships. We indicate fixed file content by uppercasing it.
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [extensions] > [extensions]
> fix = > fix =
> strip =
> [fix] > [fix]
> uppercase-whole-file:command="$PYTHON" $UPPERCASEPY > uppercase-whole-file:command="$PYTHON" $UPPERCASEPY
> uppercase-whole-file:pattern=set:** > uppercase-whole-file:pattern=set:**
> EOF > EOF
This tests the only behavior that should really be affected by obsolescence, so This tests the only behavior that should really be affected by obsolescence, so
we'll test it with evolution off and on. This only changes the revision we'll test it with evolution off and on. This only changes the revision
numbers, if all is well. numbers, if all is well.
GGGG GGGG
HHHH HHHH
KKKK KKKK
Change A was never a baserev because none of its children were to be fixed. Change A was never a baserev because none of its children were to be fixed.
$ cd .. $ cd ..
Test the --source option. We only do this with obsstore on to avoid duplicating
test code. We rely on the other tests to prove that obsolescence is not an
important factor here.
#if obsstore-on
$ hg init source-arg
$ cd source-arg
$ printf "aaaa\n" > a
$ hg commit -Am "change A"
adding a
$ printf "bbbb\n" > b
$ hg commit -Am "change B"
adding b
$ printf "cccc\n" > c
$ hg commit -Am "change C"
adding c
$ hg checkout 0
0 files updated, 0 files merged, 2 files removed, 0 files unresolved
$ printf "dddd\n" > d
$ hg commit -Am "change D"
adding d
created new head
$ hg log --graph --template '{rev} {desc}\n'
@ 3 change D
|
| o 2 change C
| |
| o 1 change B
|/
o 0 change A
Test passing 'wdir()' to --source
$ printf "xxxx\n" > x
$ hg add x
$ hg fix -s 'wdir()'
$ cat *
aaaa
dddd
XXXX
Test passing '.' to --source
$ printf "xxxx\n" > x
$ hg fix -s .
$ hg log --graph --template '{rev} {desc}\n'
@ 4 change D
|
| o 2 change C
| |
| o 1 change B
|/
o 0 change A
$ cat *
aaaa
DDDD
XXXX
$ hg strip -qf 4
$ hg co -q 3
Test passing other branch to --source
$ printf "xxxx\n" > x
$ hg add x
$ hg fix -s 2
$ hg log --graph --template '{rev} {desc}\n'
o 4 change C
|
| @ 3 change D
| |
o | 1 change B
|/
o 0 change A
$ hg cat -r 4 b c
bbbb
CCCC
$ cat *
aaaa
dddd
xxxx
$ hg strip -qf 4
Test passing multiple revisions to --source
$ hg fix -s '2 + .'
$ hg log --graph --template '{rev} {desc}\n'
@ 5 change D
|
| o 4 change C
| |
| o 1 change B
|/
o 0 change A
$ hg cat -r 4 b c
bbbb
CCCC
$ cat *
aaaa
DDDD
XXXX
$ cd ..
#endif
The --all flag should fix anything that wouldn't cause a problem if you fixed The --all flag should fix anything that wouldn't cause a problem if you fixed
it, including the working copy. Obsolete revisions are not fixed because that it, including the working copy. Obsolete revisions are not fixed because that
could cause divergence. Public revisions would cause an abort because they are could cause divergence. Public revisions would cause an abort because they are
immutable. We can fix orphans because their successors are still just orphans immutable. We can fix orphans because their successors are still just orphans
of the original obsolete parent. When obsolesence is off, we're just fixing and of the original obsolete parent. When obsolesence is off, we're just fixing and
replacing anything that isn't public. replacing anything that isn't public.
$ hg init fixall $ hg init fixall

tests/test-fix.t

revisions are not forgotten in later ones. The --base flag can be used to revisions are not forgotten in later ones. The --base flag can be used to
override this default behavior, though it is not usually desirable to do override this default behavior, though it is not usually desirable to do
so. so.
(use 'hg help -e fix' to show help for the fix extension) (use 'hg help -e fix' to show help for the fix extension)
options ([+] can be repeated): options ([+] can be repeated):
--all fix all non-public non-obsolete revisions --all fix all non-public non-obsolete revisions
--base REV [+] revisions to diff against (overrides automatic selection, --base REV [+] revisions to diff against (overrides automatic selection,
and applies to every revision being fixed) and applies to every revision being fixed)
-r --rev REV [+] revisions to fix -r --rev REV [+] revisions to fix
-s --source REV [+] fix the specified revisions and their descendants
-w --working-dir fix the working directory -w --working-dir fix the working directory
--whole always fix every line of a file --whole always fix every line of a file
(some details hidden, use --verbose to show complete help) (some details hidden, use --verbose to show complete help)
$ hg help -e fix $ hg help -e fix
fix extension - rewrite file content in changesets or working copy fix extension - rewrite file content in changesets or working copy
(EXPERIMENTAL) (EXPERIMENTAL)
Provides a command that runs configured tools on the contents of modified Provides a command that runs configured tools on the contents of modified