Patchwork D7641: rebase: use cmdutil.check_unique_argument() for --confirm/--dry-run

login
register
mail settings
Submitter phabricator
Date Dec. 13, 2019, 7:56 a.m.
Message ID <differential-rev-PHID-DREV-5w3we5r2nnw3uxsx5ubq-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43786/
State Superseded
Headers show

Comments

phabricator - Dec. 13, 2019, 7:56 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I've also updated the helper to work with the hyphenated --dry-run
  option.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  hgext/rebase.py
  mercurial/cmdutil.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 17, 2019, 7:29 a.m.
pulkit added inline comments.

INLINE COMMENTS

> cmdutil.py:272
>              if previous:
>                  raise error.Abort(
>                      _(b'cannot specify both --%s and --%s') % (previous, x)

Spotted in a later patch, need to replace `_` back to `-` before printing output.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: martinvonz, #hg-reviewers, pulkit
Cc: mercurial-devel
phabricator - Dec. 17, 2019, 3:16 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in cmdutil.py:272
> Spotted in a later patch, need to replace `_` back to `-` before printing output.

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: martinvonz, #hg-reviewers, pulkit
Cc: mercurial-devel
Yuya Nishihara - Dec. 17, 2019, 3:30 p.m.
> > Spotted in a later patch, need to replace `_` back to `-` before printing output.
> 
> Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

Maybe he meant `opts[b'dry_run']` vs `--dry-run`. `*args` should follow
underscore naming, but the printed option names don't.
phabricator - Dec. 17, 2019, 3:32 p.m.
yuja added a comment.


  >> Spotted in a later patch, need to replace `_` back to `-` before printing output.
  >
  > Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?
  
  Maybe he meant `opts[b'dry_run']` vs `--dry-run`. `*args` should follow
  underscore naming, but the printed option names don't.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: martinvonz, #hg-reviewers, pulkit
Cc: yuja, mercurial-devel
phabricator - Dec. 17, 2019, 4:02 p.m.
pulkit added a comment.


  In D7641#113027 <https://phab.mercurial-scm.org/D7641#113027>, @yuja wrote:
  
  >>> Spotted in a later patch, need to replace `_` back to `-` before printing output.
  >>
  >> Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?
  >
  > Maybe he meant `opts[b'dry_run']` vs `--dry-run`. `*args` should follow
  > underscore naming, but the printed option names don't.
  
  Yep what Yuya said. Spotted in D7644 <https://phab.mercurial-scm.org/D7644>, test-rebase-obsolete.t, line 490.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: martinvonz, #hg-reviewers, pulkit
Cc: yuja, mercurial-devel
phabricator - Dec. 17, 2019, 5:55 p.m.
martinvonz added a comment.


  In D7641#113027 <https://phab.mercurial-scm.org/D7641#113027>, @yuja wrote:
  
  >>> Spotted in a later patch, need to replace `_` back to `-` before printing output.
  >>
  >> Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?
  >
  > Maybe he meant `opts[b'dry_run']` vs `--dry-run`. `*args` should follow
  > underscore naming, but the printed option names don't.
  
  
  
  In D7641#113029 <https://phab.mercurial-scm.org/D7641#113029>, @pulkit wrote:
  
  > In D7641#113027 <https://phab.mercurial-scm.org/D7641#113027>, @yuja wrote:
  >
  >>>> Spotted in a later patch, need to replace `_` back to `-` before printing output.
  >>>
  >>> Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?
  >>
  >> Maybe he meant `opts[b'dry_run']` vs `--dry-run`. `*args` should follow
  >> underscore naming, but the printed option names don't.
  >
  > Yep what Yuya said. Spotted in D7644 <https://phab.mercurial-scm.org/D7644>, test-rebase-obsolete.t, line 490.
  
  Oh, I would consider that a bug in that patch (I would have used `dry-run` if I had noticed). However, I can see how you would prefer that we pass in the underscore-separated name (since that's what we do with indexing into `opts`). I was concerned that there were existing flags that were underscore-separated in the UI, but there doesn't seem to be any (I checked by running the test suite with a hacked `fancyopts.py`). So I'll switch to using the underscore-separated names in the API.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: martinvonz, #hg-reviewers, pulkit
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -267,7 +267,7 @@ 
     """
     previous = None
     for x in args:
-        if opts.get(x):
+        if opts.get(x.replace(b'-', b'_')):
             if previous:
                 raise error.Abort(
                     _(b'cannot specify both --%s and --%s') % (previous, x)
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1022,12 +1022,9 @@ 
     dryrun = opts.get(b'dry_run')
     confirm = opts.get(b'confirm')
     action = cmdutil.check_unique_argument(opts, b'abort', b'stop', b'continue')
-    if dryrun and action:
-        raise error.Abort(_(b'cannot specify both --dry-run and --%s') % action)
-    if confirm and action:
-        raise error.Abort(_(b'cannot specify both --confirm and --%s') % action)
-    if dryrun and confirm:
-        raise error.Abort(_(b'cannot specify both --confirm and --dry-run'))
+    if action:
+        cmdutil.check_unique_argument(opts, b'confirm', b'dry-run', action)
+    cmdutil.check_unique_argument(opts, b'confirm', b'dry-run')
 
     if action or repo.currenttransaction() is not None:
         # in-memory rebase is not compatible with resuming rebases.