Page MenuHomePhabricator

push: add option to abort on dirty working copy if parent is pushed
Needs RevisionPublic

Authored by martinvonz on Oct 15 2021, 7:52 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Summary

This patch adds a config option to make it an error to push the parent
of the working copy when the working copy is dirty. We have had this
feature enabled by default internally for a few years. I think most
users have found it helpful.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Oct 15 2021, 7:52 PM
pulkit added a subscriber: pulkit.Oct 16 2021, 2:40 PM

I am big +1 on this and will personally use it. Thanks!

mercurial/configitems.py
1869

nit: IIRC, in other configs, we use allow, warn and abort as options. Push on dirty working copy set to ignore seems a bit confusing to me.

marmoute added inline comments.
mercurial/commands.py
5644

To match other uI better we could have a --check-dirty flag that would turn the feature on and an --no-check-dirty who disable it.

With a commands.push.check-dirty (or something similar) that control the default value/behavior of this flag.

What do you think?

martinvonz marked an inline comment as done.Oct 21 2021, 1:19 PM
martinvonz updated this revision to Diff 30973.

With a commands.push.check-dirty (or something similar) that control the default value/behavior of this flag.

mercurial/commands.py
5644

To match other uI better we could have a --check-dirty flag that would turn the feature on and an --no-check-dirty who disable it.

Do you mean a tri-state flag? Where do we currently use that?

With a commands.push.check-dirty (or something similar) that control the default value/behavior of this flag.

Good point. We already have commands.update.check, which is already used by commands other than update, so even if we decide to use this new commands.push.check-dirty from other commands, it won't be any worse.

mercurial/configitems.py
1869

Makes sense.

marmoute added inline comments.Oct 21 2021, 1:27 PM
mercurial/commands.py
5644
To match other UI better we could have a --check-dirty flag that would turn the feature on and an --no-check-dirty who disable it.

Do you mean a tri-state flag? Where do we currently use that?

I am not sure what is your question here ? We allowing --no- variant for boolean flags has been around for a while and is used in various places. Including with a config option that control the default value. (e.g. hg update --merge)

spectral added inline comments.
mercurial/commands.py
5644

I believe there's two requests - a tri-state bool, and a renaming of the flag. I like the idea of the tri-state bool, but dislike the suggested name; I don't think that --check-dirty would be used often (if you remember to specify it manually, you can just run hg status -- it'd only be useful in scripts), and we might consider adding this behavior to tweakdefaults in the future; requiring --no- flags to override "default" safety features feels backwards.

Kyle message made me realize that I had not sent mine, so I'm doing that now.

mercurial/commands.py
5644

I am not sure what is your question here ? We allowing --no- variant for boolean flags has been around for a while and is used in various places.

If the default value of the flag is False, then passing --no-check-dirty is the same as not passing it. Do you mean that the default should be None (i.e. a tri-state)?

What other flag(s) were you thinking of that use --check-* rather than --allow-*?

martinvonz added inline comments.Oct 26 2021, 6:34 PM
mercurial/commands.py
5644

Kyle and I looked for existing examples of tri-state flags. The only one we could find was hg purge's --comfirm argument: If neither --confirm nor -no-confirm is passed, then the default value depends on whether the purge extension is loaded. I can make hg push behave similarly (defaulting to the config value if neither --allow-dirty nor --no-allow-dirty is passed). Is that what we want? I don't care which (except that I've already implemented it as a regular, non-tri-state flag).

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.

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.

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.

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.

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, 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
Alphare requested changes to this revision.Dec 3 2021, 10:21 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
mercurial/cmdutil.py
1118

This assumption seems false to me? For example, you can push only some drafts (if the remote already has some of them) and your parent(s) might be a secret?

I think this revset would make more sense: (parents() and ancestors(%ln)) and not secret(). You're asking the question "am I pushing my parents", regardless of phase... excluding secrets.

Also, this is done really high up and happens before the discovery, which means that you're not really working with the actual set of revisions you're pushing. This could probably cause subtle bugs with some extensions? Might as well not take the risk and do it inside exchange.push.

mercurial/commands.py
5753

See my above comment about doing this later in the exchange when we have the final set of revisions.

This revision now requires changes to proceed.Dec 3 2021, 10:21 AM

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.

marmoute added a comment.EditedDec 16 2021, 2:07 PM

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.

I see, "None" and "True" value get treated the same in that regard. It seems a small technical limitation that is fairly easy to lift. Do you have other reasons to be against them ?

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

I am curious if the warning version of this would give you most of the benefit with none of the anoyance. Did you gave it a try ? (see my previous comment about "forgetting to add a files" as a bonus)