( )⚙ D3901 histedit: add history-editing-backup config option

This is an archive of the discontinued Mercurial Phabricator instance.

histedit: add history-editing-backup config option
ClosedPublic

Authored by khanchi97 on Jul 10 2018, 7:40 AM.

Details

Summary

Instead of passing --no-backup option every time you don't
want to store backup, now you can set config option:

[ui]
history-editing-backup = False

This option aims to operate on every history editing command.

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.Jul 10 2018, 7:40 AM
khanchi97 added inline comments.Jul 10 2018, 7:47 AM
hgext/histedit.py
240

What is the right place to register this config option? Because if I register this in histedit extension then someone who doesn't have enabled histedit extension won't be able to use this config option. But this option aims to work with every history editing command.

yuja added a subscriber: yuja.Jul 11 2018, 7:27 AM

What is the right place to register this config option?

mercurial/configitems.py

FWIW, adding the [edithistory] section doesn't sound nice unless we have
more options to be added. I don't have any better idea, but we generally
add random stuff to the [ui] section.

khanchi97 edited the summary of this revision. (Show Details)Jul 11 2018, 9:12 AM
khanchi97 updated this revision to Diff 9531.
yuja added a subscriber: pulkit.Jul 18 2018, 8:34 AM

+coreconfigitem('ui', 'historyediting_backup',
+ default=True,
+)
coreconfigitem('ui', 'interactive',

default=None,

)
diff --git a/hgext/histedit.py b/hgext/histedit.py

  • a/hgext/histedit.py

+++ b/hgext/histedit.py
@@ -1111,7 +1111,8 @@

fm.startitem()
goal = _getgoal(opts)
revs = opts.get('rev', [])
  • nobackup = opts.get('no_backup')

+ nobackup = (opts.get('no_backup') or
+ not ui.configbool('ui', 'historyediting_backup'))

history-editing-backup per new rule.

https://www.mercurial-scm.org/wiki/UIGuideline#config

Can you add # experimental config: ui.history-editing-backup to silence
check-config? It's probably too late to add full support for this option
and make it documented.

And, should we drop the --no-backup option? @pulkit what do you think?

khanchi97 edited the summary of this revision. (Show Details)Jul 18 2018, 1:28 PM
khanchi97 retitled this revision from histedit: add nobackup config option to histedit: add history-editing-backup config option.
khanchi97 updated this revision to Diff 9625.

And, should we drop the --no-backup option?

IIUC,
dropped -> user won't be able to use that option
deprecated -> can use, but this is not preferred option to use
Am I right above?
If yes, why not deprecate then?

In D3901#61668, @yuja wrote:

+coreconfigitem('ui', 'historyediting_backup',
+ default=True,
+)
coreconfigitem('ui', 'interactive',

default=None,

)
diff --git a/hgext/histedit.py b/hgext/histedit.py

  • a/hgext/histedit.py

+++ b/hgext/histedit.py
@@ -1111,7 +1111,8 @@

fm.startitem()
goal = _getgoal(opts)
revs = opts.get('rev', [])
  • nobackup = opts.get('no_backup')

+ nobackup = (opts.get('no_backup') or
+ not ui.configbool('ui', 'historyediting_backup'))

history-editing-backup per new rule.
https://www.mercurial-scm.org/wiki/UIGuideline#config
Can you add # experimental config: ui.history-editing-backup to silence
check-config? It's probably too late to add full support for this option
and make it documented.
And, should we drop the --no-backup option? @pulkit what do you think?

Yep, we should either have this option everywhere or not have it anywhere. I too think that config option is better suited here. Let's drop it and comment on the bug which needed the flag about the config option.

And, should we drop the --no-backup option?

IIUC,
dropped -> user won't be able to use that option
deprecated -> can use, but this is not preferred option to use
Am I right above?

Yes, you are right above.

If yes, why not deprecate then?

Because this option has not been part of a release yet. There is no point of releasing a deprecated feature.

Because this option has not been part of a release yet. There is no point of releasing a deprecated feature.

Got it. Thanks!

And, should we drop the --no-backup option? @pulkit what do you think?

Yep, we should either have this option everywhere or not have it anywhere. I too think that config option is better suited here. Let's drop it and comment on the bug which needed the flag about the config option.

How do we drop a feature? Do I need to remove all the code related to --no-backup option?

yuja added a comment.Jul 19 2018, 8:23 AM

Queued with some typo fixes, thanks.

Do I need to remove all the code related to --no-backup option?

Yes. Remove handling of opts.get('no_backup'), and replace/remove tests
with --config ui.history-editing-backup=False.

This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Jul 20 2018, 11:02 AM
mercurial/configitems.py
1096

sorry for noticing late, but can me move it somewhere else instead of ui. I don't think ui is the right place for it. @yuja what do you think?

khanchi97 added inline comments.Jul 20 2018, 4:13 PM
mercurial/configitems.py
1096

@pulkit I do also think ui is not right place for it. And in one of my previous patch I added a new section for this config but I agree with yuya that when adding a new section we should have multiple options to fit in that section and he suggested me to add this in ui section. @pulkit what do you think could be the right place for this config?

yuja added a comment.Jul 22 2018, 1:36 AM

sorry for noticing late, but can me move it somewhere else instead of ui. I don't think ui is the right place for it. @yuja what do you think?

@pulkit I do also think ui is not right place for it. And in one of my previous patch I added a new section for this config but I agree with yuya that when adding a new section we should have multiple options to fit in that section and he suggested me to add this in ui section. @pulkit what do you think could be the right place for this config?

I don't like [ui] either, but I have no better idea so I suggested it
to get things done.

In D3901#61776, @yuja wrote:

sorry for noticing late, but can me move it somewhere else instead of ui. I don't think ui is the right place for it. @yuja what do you think?

@pulkit I do also think ui is not right place for it. And in one of my previous patch I added a new section for this config but I agree with yuya that when adding a new section we should have multiple options to fit in that section and he suggested me to add this in ui section. @pulkit what do you think could be the right place for this config?

I don't like [ui] either, but I have no better idea so I suggested it
to get things done.

Yeah, that's a nice approach. If I find something better I will send a patch for stable.