Patchwork D2934: forget: add --confirm option

login
register
mail settings
Submitter phabricator
Date March 22, 2018, 2:02 p.m.
Message ID <differential-rev-PHID-DREV-3f6gtb2ooyqikatiqpst-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29778/
State Superseded
Headers show

Comments

phabricator - March 22, 2018, 2:02 p.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Also added confirmopts in cmdutil.py

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/commands.py
  tests/test-add.t

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - March 22, 2018, 3:39 p.m.
av6 added inline comments.

INLINE COMMENTS

> cmdutil.py:2007
> +    if dryrun and confirm:
> +        raise error.Abort(_("can't specify --dry-run and --confirm"))
>      join = lambda f: os.path.join(prefix, f)

"cannot specify both --dry-run and --confirm" is the style used in other commands.

> commands.py:2043
>      '^forget',
> -    walkopts + dryrunopts,
> +    walkopts + dryrunopts + confirmopts,
>      _('[OPTION]... FILE...'), inferrepo=True)

Looks like test-completion.t also needs to be updated.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: av6, mercurial-devel
phabricator - March 22, 2018, 4:39 p.m.
pulkit requested changes to this revision.
pulkit added inline comments.

INLINE COMMENTS

> cmdutil.py:2051
>  
> +    if confirm:
> +        if ui.promptchoice(_('are you sure you want to forget (yn)?'

This should be before "removing <filename>" message. Also this message should also show filename, something like "forget <filenames> (yn)?"

> cmdutil.py:2054
> +                             '$$ &Yes $$ &No')):
> +            raise error.Abort(_('forget canceled'))
> +

I am not sure what are you trying do here. Can you look again?

> commands.py:2079
>      m = scmutil.match(repo[None], pats, opts)
> -    dryrun = opts.get(r'dry_run')
> +    dryrun, confirm = opts.get(r'dry_run'), opts.get(r'confirm')
>      rejected = cmdutil.forget(ui, repo, m, prefix="",

We don't need r'' prefix here. You can drop one before 'dry_run' too.

> test-add.t:290
> +  are you sure you want to forget (yn)? y
> +  $ hg diff
> +  diff -r e63c23eaa88a foo

In such cases, `hg status` will be a better option.

> test-add.t:302
> +  removing foo
> +  are you sure you want to forget (yn)? y
> +  $ cd ..

Looks like it's not reading the input. Look whether ui.interactive is set or not.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit
Cc: pulkit, av6, mercurial-devel
phabricator - March 23, 2018, 2:10 p.m.
yuja added a comment.


  Just curious why we want the `--confirm` option for such a simple command.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit
Cc: yuja, pulkit, av6, mercurial-devel
phabricator - March 23, 2018, 4:47 p.m.
pulkit requested changes to this revision.
pulkit added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> cmdutil.py:2047
>  
> +    if confirm:
> +        filenames = ' '.join(forget)

I think the right behavior should be to ask for each file and forget the ones which user confirmed to forget.

> test-add.t:293
> +  $ hg up -qC .
> +  $ hg forget foo --confirm << EOF
> +  > n

This is not reading 'n' as an input here and rather going for the default value. You should set `ui.interactive` to True.

> test-add.t:300
> +  R foo
> +
> +  $ cd ..

Please add more tests involving multiple files at once.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit
Cc: yuja, pulkit, av6, mercurial-devel
phabricator - March 23, 2018, 4:50 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2934#47327, @yuja wrote:
  
  > Just curious why we want the `--confirm` option for such a simple command.
  
  
  I agree with you here that this command is simple. I think @khanchi97 is picking easy and simple ones to start with.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit
Cc: yuja, pulkit, av6, mercurial-devel
phabricator - March 23, 2018, 11:55 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> pulkit wrote in cmdutil.py:2047
> I think the right behavior should be to ask for each file and forget the ones which user confirmed to forget.

+1 to the prompt per file.

And maybe options for 'all'/'none' to apply to the rest of the files that haven't been processed?  Maybe look at --interactive on commit or similar for naming on that.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit
Cc: mharbison72, yuja, pulkit, av6, mercurial-devel
phabricator - April 1, 2018, 5:25 a.m.
khanchi97 added a comment.


  I have made the requested changes. Now --confirm will prompt per file and also added all/skip option which will either include all the remaining files or skip all the remaining files according the user input.

INLINE COMMENTS

> av6 wrote in cmdutil.py:2007
> "cannot specify both --dry-run and --confirm" is the style used in other commands.

sorry, am I supposed to make any change here?

> pulkit wrote in cmdutil.py:2047
> I think the right behavior should be to ask for each file and forget the ones which user confirmed to forget.

cool :)

> av6 wrote in commands.py:2043
> Looks like test-completion.t also needs to be updated.

yeah, I will update this.

> test-add.t:302
> +  removing foo
> +  are you sure you want to forget (yn)? y
> +  $ cd ..

I need some help here. Why it's not passing `n` as a response? I passed `n` in line 299 but as a response from user why it is taking `y` everytime?

> pulkit wrote in test-add.t:302
> Looks like it's not reading the input. Look whether ui.interactive is set or not.

I have set ui.interactive=True but, the problem is still same. And also by default ui.interactive is always True.

> test-add.t:296
> +  > EOF
> +  forget foo (yn)? y
> +  removing foo

I have set ui.interactive=True but it still not read input.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit
Cc: mharbison72, yuja, pulkit, av6, mercurial-devel
phabricator - April 17, 2018, 2:33 a.m.
durin42 accepted this revision.
durin42 added a comment.


  I've touched this up in-flight and will land it, thanks.
  
  (Suggestion: diff the output of `hg export` on your version vs the one that lands to see what I needed to tweak - there were some oversights in tests during development it looks like.)

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit, durin42
Cc: durin42, mharbison72, yuja, pulkit, av6, mercurial-devel
phabricator - April 17, 2018, 3:24 a.m.
mharbison72 added a comment.


  In https://phab.mercurial-scm.org/D2934#54155, @durin42 wrote:
  
  > (Suggestion: diff the output of `hg export` on your version vs the one that lands to see what I needed to tweak - there were some oversights in tests during development it looks like.)
  
  
  Alternately if you have the extdiff extension configured, you can diff the two revisions directly and supply `--patch` to the extdiff command get the same effect.  I think it's easier to view graphically, than a diff of a diff.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit, durin42
Cc: durin42, mharbison72, yuja, pulkit, av6, mercurial-devel
phabricator - April 17, 2018, 12:46 p.m.
yuja added a comment.


  The change looks good, but new behavior sounds more like `--interactive`
  than `--confirm`.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit, durin42
Cc: durin42, mharbison72, yuja, pulkit, av6, mercurial-devel
phabricator - April 17, 2018, 1:36 p.m.
durin42 added a comment.


  I'm fine to rename this to --interactive if we think that makes more sense, even if we do it during the freeze.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit, durin42
Cc: durin42, mharbison72, yuja, pulkit, av6, mercurial-devel
phabricator - April 18, 2018, 8:29 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2934#54256, @yuja wrote:
  
  > The change looks good, but new behavior sounds more like `--interactive`
  >  than `--confirm`.
  
  
  I agree. @khanchi97 can you send a follow-up renaming the flag to `--interactive`?

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit, durin42
Cc: durin42, mharbison72, yuja, pulkit, av6, mercurial-devel
phabricator - April 18, 2018, 11:30 a.m.
khanchi97 added a comment.


  In https://phab.mercurial-scm.org/D2934#54295, @pulkit wrote:
  
  > In https://phab.mercurial-scm.org/D2934#54256, @yuja wrote:
  >
  > > The change looks good, but new behavior sounds more like `--interactive`
  > >  than `--confirm`.
  >
  >
  > I agree. @khanchi97 can you send a follow-up renaming the flag to `--interactive`?
  
  
  Okay, I will send one. I also think --interactive is better than --confirm.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, av6, pulkit, durin42
Cc: durin42, mharbison72, yuja, pulkit, av6, mercurial-devel

Patch

diff --git a/tests/test-add.t b/tests/test-add.t
--- a/tests/test-add.t
+++ b/tests/test-add.t
@@ -272,3 +272,32 @@ 
   [1]
 
   $ cd ..
+
+test --confirm option in forget
+
+  $ hg init forgetconfirm
+  $ cd forgetconfirm
+  $ echo foo > foo
+  $ hg commit -qAm "foo"
+  $ hg forget foo --dry-run --confirm
+  abort: can't specify --dry-run and --confirm
+  [255]
+  $ hg forget foo --confirm -v << EOF
+  > y
+  > EOF
+  removing foo
+  are you sure you want to forget (yn)? y
+  $ hg diff
+  diff -r e63c23eaa88a foo
+  --- a/foo	Thu Jan 01 00:00:00 1970 +0000
+  +++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  @@ -1,1 +0,0 @@
+  -foo
+
+  $ hg up -qC .
+  $ hg forget foo --confirm -v << EOF
+  > n
+  > EOF
+  removing foo
+  are you sure you want to forget (yn)? y
+  $ cd ..
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -108,6 +108,7 @@ 
 ]
 
 dryrunopts = cmdutil.dryrunopts
+confirmopts = cmdutil.confirmopts
 remoteopts = cmdutil.remoteopts
 walkopts = cmdutil.walkopts
 commitopts = cmdutil.commitopts
@@ -2039,7 +2040,7 @@ 
 
 @command(
     '^forget',
-    walkopts + dryrunopts,
+    walkopts + dryrunopts + confirmopts,
     _('[OPTION]... FILE...'), inferrepo=True)
 def forget(ui, repo, *pats, **opts):
     """forget the specified files on the next commit
@@ -2075,9 +2076,10 @@ 
         raise error.Abort(_('no files specified'))
 
     m = scmutil.match(repo[None], pats, opts)
-    dryrun = opts.get(r'dry_run')
+    dryrun, confirm = opts.get(r'dry_run'), opts.get(r'confirm')
     rejected = cmdutil.forget(ui, repo, m, prefix="",
-                              explicitonly=False, dryrun=dryrun)[0]
+                              explicitonly=False, dryrun=dryrun,
+                              confirm=confirm)[0]
     return rejected and 1 or 0
 
 @command(
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -58,6 +58,11 @@ 
      _('do not perform actions, just print output')),
 ]
 
+confirmopts = [
+    ('', 'confirm', None,
+     _('ask before applying actions')),
+]
+
 remoteopts = [
     ('e', 'ssh', '',
      _('specify ssh command to use'), _('CMD')),
@@ -1997,7 +2002,9 @@ 
         for subpath in ctx.substate:
             ctx.sub(subpath).addwebdirpath(serverpath, webconf)
 
-def forget(ui, repo, match, prefix, explicitonly, dryrun):
+def forget(ui, repo, match, prefix, explicitonly, dryrun, confirm):
+    if dryrun and confirm:
+        raise error.Abort(_("can't specify --dry-run and --confirm"))
     join = lambda f: os.path.join(prefix, f)
     bad = []
     badfn = lambda x, y: bad.append(x) or match.bad(x, y)
@@ -2013,7 +2020,8 @@ 
         sub = wctx.sub(subpath)
         try:
             submatch = matchmod.subdirmatcher(subpath, match)
-            subbad, subforgot = sub.forget(submatch, prefix, dryrun=dryrun)
+            subbad, subforgot = sub.forget(submatch, prefix,
+                                           dryrun=dryrun, confirm=confirm)
             bad.extend([subpath + '/' + f for f in subbad])
             forgot.extend([subpath + '/' + f for f in subforgot])
         except error.LookupError:
@@ -2037,9 +2045,14 @@ 
                     bad.append(f)
 
     for f in forget:
-        if ui.verbose or not match.exact(f):
+        if ui.verbose or not match.exact(f) or confirm:
             ui.status(_('removing %s\n') % match.rel(f))
 
+    if confirm:
+        if ui.promptchoice(_('are you sure you want to forget (yn)?'
+                             '$$ &Yes $$ &No')):
+            raise error.Abort(_('forget canceled'))
+
     if not dryrun:
         rejected = wctx.forget(forget, prefix)
         bad.extend(f for f in rejected if f in match.files())