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

Tue Dec 7 20:28:22 UTC 2021

spectral added a comment.

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

  The biggest reason is because there's currently, as far as I know, no support in Mercurial's flag parser code for tribools, so these won't be annotated with the `[no-]` prefix in the help text.

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

  I can't describe how many times the internal version of this check we have at Google has saved me from making a mistake. I would recommend that you check whether you truly do want to do this frequently or not :)

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

  I did not know about that and I just turned it on, because that sounds really nice. But this would not actually cover what we're trying to do, as it requires the user to remember to run commit or whatever, and that's what we're trying to check that they did :P

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

  I guess this comes down to whether we expect this in tweakdefaults or not; regardless of whether it's actually going to be in tweakdefaults, we're going to enable it at Google for everyone. When combined with my preference for not having flag negations, we end up with the --allow wording, and that's what we currently have it named internally. I really think it *should* be added to tweakdefaults, but I'll admit that my own personal usage of Mercurial might not match what non-Google users of Mercurial do, and maybe this causes issues with some workflows there.

  Like I said before, I also think it's going to be exceedingly rare, if this isn't part of tweakdefaults, for someone to *enable* on the commandline. No one is going to start typing `hg push --check`, they'll either forget to specify it, or they'll turn on the option somehow. So in my mind the flag is purely an "escape hatch" for "no, I really want to do this", and the word "allow" conveys that quite well.

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, Alphare
Cc: Alphare, spectral, marmoute, pulkit, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mercurial-scm.org/pipermail/mercurial-patches/attachments/20211207/4c3eeb9a/attachment.html>