Page MenuHomePhabricator

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

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

Details

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