Patchwork D8284: fix: disallow `hg fix --all --working-dir`

login
register
mail settings
Submitter phabricator
Date March 13, 2020, 7:33 p.m.
Message ID <differential-rev-PHID-DREV-talrfujrx4pgmsl7d77c-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45767/
State Superseded
Headers show

Comments

phabricator - March 13, 2020, 7:33 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8284

AFFECTED FILES
  hgext/fix.py
  tests/test-fix-topology.t

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - March 14, 2020, 8:41 a.m.
marmoute added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8284/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8284

To: martinvonz, #hg-reviewers, mharbison72
Cc: marmoute, mercurial-devel
phabricator - March 16, 2020, 5:36 p.m.
martinvonz added a comment.


  In D8284#123838 <https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8284/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8284

To: martinvonz, #hg-reviewers, mharbison72
Cc: marmoute, mercurial-devel
phabricator - March 16, 2020, 8:15 p.m.
marmoute added a comment.


  In D8284#123872 <https://phab.mercurial-scm.org/D8284#123872>, @martinvonz wrote:
  
  > In D8284#123838 <https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8284/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8284

To: martinvonz, #hg-reviewers, mharbison72
Cc: marmoute, mercurial-devel
phabricator - March 17, 2020, 12:31 a.m.
martinvonz added a comment.


  In D8284#123873 <https://phab.mercurial-scm.org/D8284#123873>, @marmoute wrote:
  
  > In D8284#123872 <https://phab.mercurial-scm.org/D8284#123872>, @martinvonz wrote:
  >
  >> In D8284#123838 <https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8284/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8284

To: martinvonz, #hg-reviewers, mharbison72
Cc: marmoute, mercurial-devel
phabricator - March 17, 2020, 1:56 a.m.
mharbison72 added a comment.


  In D8284#123877 <https://phab.mercurial-scm.org/D8284#123877>, @martinvonz wrote:
  
  > In D8284#123873 <https://phab.mercurial-scm.org/D8284#123873>, @marmoute wrote:
  >
  >> In D8284#123872 <https://phab.mercurial-scm.org/D8284#123872>, @martinvonz wrote:
  >>
  >>> In D8284#123838 <https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8284/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8284

To: martinvonz, #hg-reviewers, mharbison72
Cc: marmoute, mercurial-devel

Patch

diff --git a/tests/test-fix-topology.t b/tests/test-fix-topology.t
--- a/tests/test-fix-topology.t
+++ b/tests/test-fix-topology.t
@@ -271,6 +271,9 @@ 
 
   $ hg init fixall
   $ cd fixall
+  $ hg fix --all --working-dir
+  abort: cannot specify both --working-dir and --all
+  [255]
 
 #if obsstore-on
   $ printf "one\n" > foo.whole
diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -250,6 +250,7 @@ 
     """
     opts = pycompat.byteskwargs(opts)
     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']:
         opts[b'rev'] = [b'not public() and not obsolete()']
         opts[b'working_dir'] = True