[Updated] D11680: push: add option to abort on dirty working copy if parent is pushed

[marmoute (Pierre-Yves David)]

marmoute added a comment.


  In D11680#180023 <https://phab.mercurial-scm.org/D11680#180023>, @spectral wrote:
  
  > In D11680#179988 <https://phab.mercurial-scm.org/D11680#179988>, @marmoute wrote:
  >
  >> The ability to use `--no-` variants is fairly new (a couple of year maybe ?), but I (and others) have been trying to put it to good use since then. Since is quite new I don't think they so many of of such flag yet, but a notable example of that approach is `--merge` that I use daily.
  >
  > As in `hg update --no-merge`? I don't see how that does anything, I might be missing something?
  >
  >   @command(
  >       b'update|up|checkout|co',
  >       [... (b'm', b'merge', None, _(b'merge uncommitted changes')), ...]
  >   )
  >   def update(ui, repo, node=None, **opts):
  >       ...
  >       merge = opts.get('merge')
  >       ...
  >       if check:
  >           updatecheck = b'abort'
  >       elif merge:  # Truthiness check only
  >           updatecheck = b'none'
  >
  > The only way that might do something is if you have something in [defaults] that sets --merge to true, such that this overrides it.
  
  You are right, I expected this to work but this wasn't implemented, the `--merge` properly override the config, but `--no-merge` did not override the other way around.
  
  So I fixed it in D11851 <https://phab.mercurial-scm.org/D11851>, and it now does.
  
  >> I agree that in this case one of the variants seems less useful as the other, but it does not seem useless either. For example it has obvious usage in script and alias. Having it can also help to build a more consistent experience around boolean flag. (and who knows what users would actually use)
  >> Regarding `check` vs `allow`, I find `no-check` better than `no-allow` (that I feel a bit awkward). Regarding consistency with existing options, `hg update` has as notable `--check` flag, that check for a dirty working directory.
  >
  > I'm trying us to the most desirable end state, which I hope is that this gets added to tweakdefaults, at which point we'd have the following scenarios:
  >
  > - tweakdefaults enabled: user runs `hg push` and it fails, telling the user to run `hg push --allow-dirty`; they do so. If they hit this frequently, they either disable this config knob or learn to type `hg push --allow`
  > - tweakdefaults not enabled [current state]: user runs `hg push` and it succeeds, uploads even though they have a dirty working directory. Never learns about the config option.
  > - script (which should be using HGPLAIN=1 to disable tweakdefaults): `hg push` behaves like before, `hg push --no-allow-dirty` works if they actually want that behavior.
  > - aliases: I imagine it'll be rare that people make an alias like `safepush = push --no-allow-dirty`, but this is still something that the user does very infrequently, while I expect the interactive typing to be more common
  >
  > In this set of cases, the first one is the one I think we should be optimizing for. The other three are cases where writing it like `--check-dirty` makes sense, but I would argue that it doesn't matter that there's more cases, because the number of times it happens (for scripts and aliases) is much lower, and the interactive user in the second case would be much better served by enabling tweakdefaults (or at least this config option).
  > In my opinion, flag negations should be rare. We also have the problem that if we're actually creating a tri-state boolean, we currently do not have any mechanism in the flag parsing code to show those properly in the help text. This will show up as:
  >
  >   --allow-dirty                  allow pushing with a dirty working copy, overriding
  >                                  commands.push.check-dirty=abort (EXPERIMENTAL)
  >
  > with no `[no-]` prefix, because it's not recognized as a tribool.
  
  I can see multiple questions here with an agreement level varying from one question to another. So let me try to split them apart for clarity.
  
  Should there be a config for this behavior ?
  --------------------------------------------
  
  In my opinion : Yes,
  (and I think we have a consensus)
  
  Should this be a tri-state option ? (True/None/False; that override the config value when ≠ None)
  -------------------------------------------------------------------------------------------------
  
  In my opinion : Yes, that seems more logical and make good use of `--no-prefix`
  
  (I am unclear about what the consensus currently is)
  
  Should Flag negation be rare ?
  ------------------------------
  
  I disagree here. We should use clearest available verb for flags, using negation to get the meaning we need. I would draw a similarity with boolean naming in the code, where we don't have a preferred "default" state for the common case, but we phrase the boolean in a way that is easy to read.
  
  In addition, having a mix of negation and non-negation as common case will help make the (negation) feature more discoverable.
  
  What's your rationale for wanting them to be rare ?
  
  Should the proposed behavior be part of tweakdefault ?
  ------------------------------------------------------
  
  I don't think so, `hg push` is reasonably independent from the working copy and it is a common use-case to push with uncommitted changes (pushing the working copy parent or not). Making it disabled by default seems weird to me.
  
  A related behavior/config is `commands.commit.post-status` that display information about dirty content after the commit (including about unknown files !). I feel like it could catch most¹ of the error case you are trying to catch here (or at least, it would in my case). With a much smoother impact on the common workflow.
  
  [1] even more since it would catch the "forgot to add a file" case.
  
  Should we have a "warning" version of this behavior ?
  -----------------------------------------------------
  
  In my opinion : Yes, it brings most of the value with very few drawbacks.
  
  Should we use `--allow-dirty-wc` or `--no-check-dirty-wc`.
  ----------------------------------------------------------
  
  I prefer the `--check` wording :
  
  - it is more consistent with `hg update --check`,
  - the `--no-check` version is clearer that a validation step have been explicitly dropped

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D11680/new/

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

To: martinvonz, #hg-reviewers
Cc: spectral, marmoute, pulkit, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mercurial-scm.org/pipermail/mercurial-patches/attachments/20211203/c1df266c/attachment.html>