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

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3901

histedit: add history-editing-backup config option
ClosedPublic

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

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGc2586a6e5884: histedit: add history-editing-backup config option
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
Lint Skipped
Unit
Unit Tests Skipped

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.
khanchi97 added a comment.Jul 18 2018, 1:45 PM

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?

pulkit added a comment.Jul 18 2018, 3:14 PM
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.

pulkit added a comment.Jul 18 2018, 3:16 PM
In D3901#61681, @khanchi97 wrote:

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.

khanchi97 added a comment.Jul 18 2018, 3:56 PM

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

Got it. Thanks!

khanchi97 added a comment.Jul 18 2018, 4:01 PM

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.

Closed by commit rHGc2586a6e5884: histedit: add history-editing-backup config option (authored by khanchi97). · Explain WhyJul 19 2018, 8:28 AM
This revision was automatically updated to reflect the committed changes.
pulkit added inline comments.Jul 20 2018, 11:02 AM
mercurial/configitems.py
1096 ↗(On Diff #9632)

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 ↗(On Diff #9632)

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

pulkit added a comment.Jul 23 2018, 2:39 PM
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.

Revision Contents
Changeset List

PathPackages
Mhgext/histedit.py (5 lines)
Mtests/test-histedit-no-backup.t (38 lines)

Diff 9493

hgext/histedit.py

default=False, default=False,
) )
configitem('histedit', 'linelen', configitem('histedit', 'linelen',
default=80, default=80,
) )
configitem('histedit', 'singletransaction', configitem('histedit', 'singletransaction',
default=False, default=False,
) )
configitem('edithistory', 'nobackup',
default=False,
)
khanchi97AuthorUnsubmitted
Not Done

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.

khanchi97: What is the right place to register this config option? Because if I register this in histedit…
# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
# be specifying the version(s) of Mercurial they are tested with, or # be specifying the version(s) of Mercurial they are tested with, or
# leave the attribute unspecified. # leave the attribute unspecified.
testedwith = 'ships-with-hg-core' testedwith = 'ships-with-hg-core'
actiontable = {} actiontable = {}
_('histedit requires exactly one ancestor revision')) _('histedit requires exactly one ancestor revision'))
def _histedit(ui, repo, state, *freeargs, **opts): def _histedit(ui, repo, state, *freeargs, **opts):
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
fm = ui.formatter('histedit', opts) fm = ui.formatter('histedit', opts)
fm.startitem() fm.startitem()
goal = _getgoal(opts) goal = _getgoal(opts)
revs = opts.get('rev', []) revs = opts.get('rev', [])
nobackup = opts.get('no_backup') nobackup = opts.get('no_backup') or ui.configbool('edithistory', 'nobackup')
rules = opts.get('commands', '') rules = opts.get('commands', '')
state.keep = opts.get('keep', False) state.keep = opts.get('keep', False)
_validateargs(ui, repo, state, freeargs, opts, goal, rules, revs) _validateargs(ui, repo, state, freeargs, opts, goal, rules, revs)
# rebuild state # rebuild state
if goal == goalcontinue: if goal == goalcontinue:
state.read() state.read()

tests/test-histedit-no-backup.t

| three | three
| |
o 1 6153eb23e623 1970-01-01 00:00 +0000 test o 1 6153eb23e623 1970-01-01 00:00 +0000 test
| two | two
| |
o 0 36b4bdd91f5b 1970-01-01 00:00 +0000 test o 0 36b4bdd91f5b 1970-01-01 00:00 +0000 test
one one
===========================
Test nobackup config option|
===========================
Test when `nobackup` config option is not enabled:
$ hg histedit -r '36b4bdd91f5b' --commands - << EOF
> pick 36b4bdd91f5b 0 one
> pick 6153eb23e623 1 two
> roll 80d23dfa866d 2 three
> edit 7d5187087c79 3 four
> EOF
merging file
Editing (7d5187087c79), you may commit or record as needed now.
(hg histedit --continue to resume)
[1]
$ hg histedit --abort
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
saved backup bundle to $TESTTMP/foo/.hg/strip-backup/1d8f701c7b35-cf7be322-backup.hg
saved backup bundle to $TESTTMP/foo/.hg/strip-backup/5c0056670bce-b54b65d0-backup.hg
Test when `nobackup` config option is enabled
Enable config option:
$ cat >>$HGRCPATH <<EOF
> [edithistory]
> nobackup=True
> EOF
$ hg histedit -r '36b4bdd91f5b' --commands - << EOF
> pick 36b4bdd91f5b 0 one
> pick 6153eb23e623 1 two
> roll 80d23dfa866d 2 three
> edit 7d5187087c79 3 four
> EOF
merging file
Editing (7d5187087c79), you may commit or record as needed now.
(hg histedit --continue to resume)
[1]
$ hg histedit --abort
1 files updated, 0 files merged, 0 files removed, 0 files unresolved