( )⚙ D2934 forget: add --confirm option

This is an archive of the discontinued Mercurial Phabricator instance.

forget: add --confirm option
ClosedPublic

Authored by khanchi97 on Mar 22 2018, 10:01 AM.

Details

Summary

Also added confirmopts in cmdutil.py

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

khanchi97 created this revision.Mar 22 2018, 10:01 AM
av6 added a subscriber: av6.Mar 22 2018, 11:39 AM
av6 added inline comments.
mercurial/cmdutil.py
2032

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

mercurial/commands.py
2064

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

av6 requested changes to this revision.Mar 22 2018, 11:39 AM
This revision now requires changes to proceed.Mar 22 2018, 11:39 AM
pulkit requested changes to this revision.Mar 22 2018, 12:38 PM
pulkit added a subscriber: pulkit.
pulkit added inline comments.
mercurial/cmdutil.py
2102

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

2105

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

mercurial/commands.py
2100

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

tests/test-add.t
290

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

302

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

khanchi97 updated this revision to Diff 7254.Mar 22 2018, 3:49 PM
yuja added a subscriber: yuja.Mar 23 2018, 10:10 AM

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

pulkit requested changes to this revision.Mar 23 2018, 12:47 PM
pulkit added inline comments.
mercurial/cmdutil.py
2072

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

tests/test-add.t
293

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

300

Please add more tests involving multiple files at once.

This revision now requires changes to proceed.Mar 23 2018, 12:47 PM
In 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.

mharbison72 added inline comments.
mercurial/cmdutil.py
2072

+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.

khanchi97 updated this revision to Diff 7285.Mar 26 2018, 7:13 AM

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.

mercurial/cmdutil.py
2032

sorry, am I supposed to make any change here?

2072

cool :)

mercurial/commands.py
2064

yeah, I will update this.

tests/test-add.t
296

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

302

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?

302

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

durin42 accepted this revision.Apr 16 2018, 10:33 PM
durin42 added a subscriber: durin42.

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.)

This revision was automatically updated to reflect the committed changes.

(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.

yuja added a comment.Apr 17 2018, 8:46 AM

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

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

In 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?

In D2934#54295, @pulkit wrote:
In 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.