Patchwork D3901: histedit: add nobackup config option

login
register
mail settings
Submitter phabricator
Date July 10, 2018, 11:40 a.m.
Message ID <differential-rev-PHID-DREV-zgrgy2dn3wr2yr73y6om-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32733/
State Superseded
Headers show

Comments

phabricator - July 10, 2018, 11:40 a.m.
khanchi97 created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Instead of passing --no-backup option every time you don't
  want to store backup, now you can set config option:
  
  [edithistory]
  nobackup = True
  
  This option aims to operate on every history editing command.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/histedit.py
  tests/test-histedit-no-backup.t

CHANGE DETAILS




To: khanchi97, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - July 10, 2018, 11:47 a.m.
khanchi97 added inline comments.

INLINE COMMENTS

> histedit.py:240
> +    default=False,
> +)
>  

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.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - July 11, 2018, 11:26 a.m.
> 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.
phabricator - July 11, 2018, 11:27 a.m.
yuja added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - July 18, 2018, 5:45 p.m.
khanchi97 added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - July 18, 2018, 7:14 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - July 18, 2018, 7:16 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - July 18, 2018, 7:56 p.m.
khanchi97 added a comment.


  > Because this option has not been part of a release yet. There is no point of releasing a deprecated feature.
  
  Got it. Thanks!

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - July 18, 2018, 8:01 p.m.
khanchi97 added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
Yuya Nishihara - July 19, 2018, 12:23 p.m.
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`.
phabricator - July 19, 2018, 12:23 p.m.
yuja added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - July 20, 2018, 3:02 p.m.
pulkit added inline comments.

INLINE COMMENTS

> configitems.py:1096
>  )
> +coreconfigitem('ui', 'history-editing-backup',
> +    default=True,

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?

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - July 20, 2018, 8:13 p.m.
khanchi97 added inline comments.

INLINE COMMENTS

> pulkit wrote in 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?

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

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
Yuya Nishihara - July 22, 2018, 5:35 a.m.
> > 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.
phabricator - July 22, 2018, 5:36 a.m.
yuja added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: khanchi97, durin42, #hg-reviewers
Cc: pulkit, yuja, mercurial-devel
phabricator - July 23, 2018, 6:39 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-histedit-no-backup.t b/tests/test-histedit-no-backup.t
--- a/tests/test-histedit-no-backup.t
+++ b/tests/test-histedit-no-backup.t
@@ -93,3 +93,41 @@ 
   o  0   36b4bdd91f5b   1970-01-01 00:00 +0000   test
        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
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -235,6 +235,9 @@ 
 configitem('histedit', 'singletransaction',
     default=False,
 )
+configitem('edithistory', 'nobackup',
+    default=False,
+)
 
 # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
 # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
@@ -1111,7 +1114,7 @@ 
     fm.startitem()
     goal = _getgoal(opts)
     revs = opts.get('rev', [])
-    nobackup = opts.get('no_backup')
+    nobackup = opts.get('no_backup') or ui.configbool('edithistory', 'nobackup')
     rules = opts.get('commands', '')
     state.keep = opts.get('keep', False)