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

Authored by spectral on Feb 2 2018, 6:40 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
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).

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped
spectral created this revision.Feb 2 2018, 6:40 PM

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

tests/test-commit.t
841–843

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)

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

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?

@spectral: what's the state of this series? Do you plan to submit a follow-up?

@spectral: what's the state of this series? Do you plan to submit a follow-up?

I think he's waiting for me to hopefully do it in a more generic way. I have been working on that. Turned out that in order to create a generic utility for moving phase, bookmarks, obsmarkers, we needed to be able to do that during interrupted rebase/histedit. So I'm first making rebase and histedit incrementally move bookmarks and add obsmarkers. I've been making good progress on that and can hopefully have a series out in a day or two (been blocked by other stuff).

If you get tired of waiting, I'm perfectly fine with getting this series in in some form first. I think it would be good to do that without the --no-secret since I found it a little odd. Perhaps just make it experimental?

@spectral: what's the state of this series? Do you plan to submit a follow-up?

I think he's waiting for me to hopefully do it in a more generic way. I have been working on that. Turned out that in order to create a generic utility for moving phase, bookmarks, obsmarkers, we needed to be able to do that during interrupted rebase/histedit. So I'm first making rebase and histedit incrementally move bookmarks and add obsmarkers. I've been making good progress on that and can hopefully have a series out in a day or two (been blocked by other stuff).

If you get tired of waiting, I'm perfectly fine with getting this series in in some form first. I think it would be good to do that without the --no-secret since I found it a little odd. Perhaps just make it experimental?

Yep, waiting :) I don't see a strong reason to get it in using this mechanism if we're going to have a better mechanism soon.

spectral abandoned this revision.Jun 22 2018, 2:30 PM