Patchwork D2013: commit: allow --no-secret to override phases.new-commit setting

login
register
mail settings
Submitter phabricator
Date Feb. 2, 2018, 11:40 p.m.
Message ID <differential-rev-PHID-DREV-r2igxcofj3mspdtp4yat-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27246/
State New
Headers show

Comments

phabricator - Feb. 2, 2018, 11:40 p.m.
spectral created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously, if the user had phases.new-commit=secret, there was no way on the
  command line to commit as draft. This lets the user specify --no-secret to
  request a draft-phase commit (which will work as long as the parent is not
  secret; if the parent is secret, the new commit will be as well regardless of
  --no-secret being specified).

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/commands.py
  tests/test-commit.t

CHANGE DETAILS




To: spectral, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 3, 2018, 6:04 a.m.
martinvonz added a comment.


  I wonder if we should instead have a --draft option for this. Reasons:
  
  - If we ever add a fourth phase (like Jun's proposed "archived" phase), then --no-secret doesn't clearly indicate "draft", it could just as well be "archived".
  - Actually, we of course already do have a third phase. One could imagine a "hg commit --public", although that's probably not useful enough to warrant its own option, but it seems to suggest that "--no-secret" doesn't necessarily mean "draft".
  - I find this tri-state boolean weird. "--secret" kind of defaults to off, but it can be made "more off" with "--no-secret".

REPOSITORY
  rHG Mercurial

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

To: spectral, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Feb. 5, 2018, 4:39 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2013#33867, @martinvonz wrote:
  
  > I wonder if we should instead have a --draft option for this. Reasons:
  >
  > - If we ever add a fourth phase (like Jun's proposed "archived" phase), then --no-secret doesn't clearly indicate "draft", it could just as well be "archived".
  > - Actually, we of course already do have a third phase. One could imagine a "hg commit --public", although that's probably not useful enough to warrant its own option, but it seems to suggest that "--no-secret" doesn't necessarily mean "draft".
  > - I find this tri-state boolean weird. "--secret" kind of defaults to off, but it can be made "more off" with "--no-secret".
  
  
  I realized later that I had not considered the ordering of phases. If we did have a --draft option and the user used it on top of a secret commit, would it be surprising if it became a secret commit? That's how the phases.new-commit=draft config works, but perhaps it's more surprising when specified on the command line. OTOH, perhaps it's similarly surprising if --no-secret creates a secret commit.

INLINE COMMENTS

> test-commit.t:841-843
> +  $ hg --config phases.new-commit=secret commit -qAm 'force draft' --no-secret
> +  $ hg phase -r .
> +  0: draft

Might be good to have a third case where we use --no-secret on top of a secret commit (and the new commit would still be secret, I assume)

REPOSITORY
  rHG Mercurial

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

To: spectral, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Feb. 5, 2018, 8:59 p.m.
spectral added a comment.


  In https://phab.mercurial-scm.org/D2013#33867, @martinvonz wrote:
  
  > I wonder if we should instead have a --draft option for this. Reasons:
  >
  > - If we ever add a fourth phase (like Jun's proposed "archived" phase), then --no-secret doesn't clearly indicate "draft", it could just as well be "archived".
  > - Actually, we of course already do have a third phase. One could imagine a "hg commit --public", although that's probably not useful enough to warrant its own option, but it seems to suggest that "--no-secret" doesn't necessarily mean "draft".
  > - I find this tri-state boolean weird. "--secret" kind of defaults to off, but it can be made "more off" with "--no-secret".
  
  
  Yeah, I wasn't sure I liked it when writing it, and I'm fine with changing it, but do we really want a proliferation of flags?  Perhaps we want a generic 'phase' flag, so one can specify -s or --secret (BC and it's the "most common" case), and --phase <public|draft|secret|archived> for more advanced use-cases?  This runs into a couple small problems, specifically that there's now "more than one way to do it [specify secret]", it's a lot of typing, and I don't think we should abbreviate it `-p` (it just doesn't feel like it's going to be common enough to warrant any abbreviation, let alone 'p', which could stand for phase, or patch, or path, or a number of other words that we might eventually add to `commit` and be sad about not being able to abbreviate in the obvious fashion)

REPOSITORY
  rHG Mercurial

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

To: spectral, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Feb. 6, 2018, 9:26 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2013#34320, @spectral wrote:
  
  > In https://phab.mercurial-scm.org/D2013#33867, @martinvonz wrote:
  >
  > > I wonder if we should instead have a --draft option for this. Reasons:
  > >
  > > - If we ever add a fourth phase (like Jun's proposed "archived" phase), then --no-secret doesn't clearly indicate "draft", it could just as well be "archived".
  > > - Actually, we of course already do have a third phase. One could imagine a "hg commit --public", although that's probably not useful enough to warrant its own option, but it seems to suggest that "--no-secret" doesn't necessarily mean "draft".
  > > - I find this tri-state boolean weird. "--secret" kind of defaults to off, but it can be made "more off" with "--no-secret".
  >
  >
  > Yeah, I wasn't sure I liked it when writing it, and I'm fine with changing it, but do we really want a proliferation of flags?  Perhaps we want a generic 'phase' flag, so one can specify -s or --secret (BC and it's the "most common" case), and --phase <public|draft|secret|archived> for more advanced use-cases?  This runs into a couple small problems, specifically that there's now "more than one way to do it [specify secret]", it's a lot of typing, and I don't think we should abbreviate it `-p` (it just doesn't feel like it's going to be common enough to warrant any abbreviation, let alone 'p', which could stand for phase, or patch, or path, or a number of other words that we might eventually add to `commit` and be sad about not being able to abbreviate in the obvious fashion)
  
  
  I think "--phase <phase>" makes sense, but it is a little long as you said. Since the real goal of this series is to preserve the phase on `hg split`, I'll see if I can do that by adding general support to scmutil.cleanupnodes() instead (Kyle and I already talked about this offline).

REPOSITORY
  rHG Mercurial

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

To: spectral, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Feb. 16, 2018, 5:48 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2013#34509, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/D2013#34320, @spectral wrote:
  >
  > > In https://phab.mercurial-scm.org/D2013#33867, @martinvonz wrote:
  > >
  > > > I wonder if we should instead have a --draft option for this. Reasons:
  > > >
  > > > - If we ever add a fourth phase (like Jun's proposed "archived" phase), then --no-secret doesn't clearly indicate "draft", it could just as well be "archived".
  > > > - Actually, we of course already do have a third phase. One could imagine a "hg commit --public", although that's probably not useful enough to warrant its own option, but it seems to suggest that "--no-secret" doesn't necessarily mean "draft".
  > > > - I find this tri-state boolean weird. "--secret" kind of defaults to off, but it can be made "more off" with "--no-secret".
  > >
  > >
  > > Yeah, I wasn't sure I liked it when writing it, and I'm fine with changing it, but do we really want a proliferation of flags?  Perhaps we want a generic 'phase' flag, so one can specify -s or --secret (BC and it's the "most common" case), and --phase <public|draft|secret|archived> for more advanced use-cases?  This runs into a couple small problems, specifically that there's now "more than one way to do it [specify secret]", it's a lot of typing, and I don't think we should abbreviate it `-p` (it just doesn't feel like it's going to be common enough to warrant any abbreviation, let alone 'p', which could stand for phase, or patch, or path, or a number of other words that we might eventually add to `commit` and be sad about not being able to abbreviate in the obvious fashion)
  >
  >
  > I think "--phase <phase>" makes sense, but it is a little long as you said. Since the real goal of this series is to preserve the phase on `hg split`, I'll see if I can do that by adding general support to scmutil.cleanupnodes() instead (Kyle and I already talked about this offline).
  
  
  Here's an update on this work:
  
  It turned out to be harder than I expected. The main problem seems to be that interrupted `hg rebase/histedit` does not call cleanupnodes, but they still expect the phase to get set. They also currently don't create obsmarkers. The reason they don't is because obsmarkers can't be undone, which is important for `hg rebase/histedit --abort`. However, these days repair.py will strip obsmarkers, so my plan now is this:
  
  1. Make rebase/histedit add obsmarkers as we go and undo them on abort [1]. This means that the user will see which commits have already been rebased when rebase/histedit is interrupted. I think that's a good thing.
  2. Make cleanupnodes() propagate phase to successor (without affecting parent commit, of course, so if parent is secret, then so will the successor be, even if the precursor was draft). This will be some new version of cleanupnodes() that will not strip the precursors in the obsmarker-less case.
  3. Remove code for propagating phase from existing commands (e.g. amend)
  
  Perhaps I'll find some time to work on this during the sprint, but it does seem a little big to be completed during the sprint.
  
  [1] It seems like we should do the same with bookmarks -- move them as we go, but move them back on abort. However, I'm not sure if that's BC. Can we pretend that it was a bug that they didn't? Maybe we don't care much about BC of interrupted rebase/histedit?

REPOSITORY
  rHG Mercurial

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

To: spectral, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/tests/test-commit.t b/tests/test-commit.t
--- a/tests/test-commit.t
+++ b/tests/test-commit.t
@@ -832,3 +832,16 @@ 
 
   $ cd ..
 
+Test that --secret/--no-secret make secret/draft commits, regardless of
+phases.new-commit
+
+  $ hg init phasetest
+  $ cd phasetest
+  $ echo foo > foo
+  $ hg --config phases.new-commit=secret commit -qAm 'force draft' --no-secret
+  $ hg phase -r .
+  0: draft
+  $ echo foo2 >> foo
+  $ hg --config phases.new-commit=draft commit -qAm 'force secret' --secret
+  $ hg phase -r .
+  1: secret
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1582,8 +1582,9 @@ 
     else:
         def commitfunc(ui, repo, message, match, opts):
             overrides = {}
-            if opts.get('secret'):
-                overrides[('phases', 'new-commit')] = 'secret'
+            if opts.get('secret') is not None:
+                overrides[('phases', 'new-commit')] = (
+                    'secret' if opts.get('secret') else 'draft')
 
             baseui = repo.baseui
             with baseui.configoverride(overrides, 'commit'):