This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8284

fix: disallow `hg fix --all --working-dir`
ClosedPublic

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

Details

Reviewers
mharbison72
Group Reviewers
hg-reviewers
Commits
rHG40f4a75938ba: fix: disallow `hg fix --all --working-dir`
Summary

--all implies --working-dir, so it's probably a mistake if the
user uses both.

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
mharbison72 accepted this revision.Mar 13 2020, 6:08 PM
marmoute added a subscriber: marmoute.Mar 14 2020, 4:41 AM

If --working-dir and --all are redundant, I don't see anyharm in allowing both to be passed.

martinvonz added a comment.Mar 16 2020, 1:36 PM
In D8284#123838, @marmoute wrote:

If --working-dir and --all are redundant, I don't see anyharm in allowing both to be passed.

The idea was to inform users that they're doing something that's a little weird, in case they were hoping for it to do something else. I don't care much and I'm fine with dropping this patch if that's the consensus.

marmoute added a comment.Mar 16 2020, 4:15 PM
In D8284#123872, @martinvonz wrote:
In D8284#123838, @marmoute wrote:

If --working-dir and --all are redundant, I don't see anyharm in allowing both to be passed.

The idea was to inform users that they're doing something that's a little weird, in case they were hoping for it to do something else. I don't care much and I'm fine with dropping this patch if that's the consensus.

If the intend is to inform, maybe we could issue a warning?

martinvonz added a comment.Mar 16 2020, 8:31 PM
In D8284#123873, @marmoute wrote:
In D8284#123872, @martinvonz wrote:
In D8284#123838, @marmoute wrote:

If --working-dir and --all are redundant, I don't see anyharm in allowing both to be passed.

The idea was to inform users that they're doing something that's a little weird, in case they were hoping for it to do something else. I don't care much and I'm fine with dropping this patch if that's the consensus.

If the intend is to inform, maybe we could issue a warning?

Makes sense. I suppose we should do the same for --all and --rev REV in that case. We currently consider that an error. I don't care much about this issue, so I'll just move this patch to the side for now. That way the rest of the stack (which I care about) can be queued independently.

mharbison72 added a comment.Mar 16 2020, 9:56 PM
In D8284#123877, @martinvonz wrote:
In D8284#123873, @marmoute wrote:
In D8284#123872, @martinvonz wrote:
In D8284#123838, @marmoute wrote:

If --working-dir and --all are redundant, I don't see anyharm in allowing both to be passed.

The idea was to inform users that they're doing something that's a little weird, in case they were hoping for it to do something else. I don't care much and I'm fine with dropping this patch if that's the consensus.

If the intend is to inform, maybe we could issue a warning?

Makes sense. I suppose we should do the same for --all and --rev REV in that case. We currently consider that an error. I don't care much about this issue, so I'll just move this patch to the side for now. That way the rest of the stack (which I care about) can be queued independently.

I can't think of any existing precedent to follow, and it would probably be fine to error out or silently ignore it. But warning here feels weird- what's the user supposed to do after the command has run? There's already enough grief with the did you mean bookmark? warning, and that's something that the user could react to.

martinvonz added a commit: rHG40f4a75938ba: fix: disallow `hg fix --all --working-dir`.Mar 16 2020, 11:03 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG40f4a75938ba: fix: disallow `hg fix --all --working-dir` (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/fix.py (1 line)
Mtests/test-fix-topology.t (3 lines)

Diff 20803

hgext/fix.py

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'])
if opts[b'all']: if opts[b'all']:
opts[b'rev'] = [b'not public() and not obsolete()'] opts[b'rev'] = [b'not public() and not obsolete()']
opts[b'working_dir'] = True opts[b'working_dir'] = True
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

tests/test-fix-topology.t

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
$ cd fixall $ cd fixall
$ hg fix --all --working-dir
abort: cannot specify both --working-dir and --all
[255]
#if obsstore-on #if obsstore-on
$ printf "one\n" > foo.whole $ printf "one\n" > foo.whole
$ hg commit -Aqm "first" $ hg commit -Aqm "first"
$ hg phase --public $ hg phase --public
$ hg tag --local root $ hg tag --local root
$ printf "two\n" > foo.whole $ printf "two\n" > foo.whole
$ hg commit -m "second" $ hg commit -m "second"