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.
Details
Details
- Reviewers
Alphare - Group Reviewers
hg-reviewers
Diff Detail
Diff Detail
- Repository
- rHG Mercurial
- Branch
- default
- Lint
No Linters Available - Unit
No Unit Test Coverage
Event Timeline
Comment Actions
I am big +1 on this and will personally use it. Thanks!
| mercurial/configitems.py | ||
|---|---|---|
| 1874 | 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. | |
| mercurial/commands.py | ||
|---|---|---|
| 5638 | 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? | |
Comment Actions
With a commands.push.check-dirty (or something similar) that control the default value/behavior of this flag.
| mercurial/commands.py | ||
|---|---|---|
| 5638 |
Do you mean a tri-state flag? Where do we currently use that?
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 | ||
| 1874 | Makes sense. | |
| mercurial/commands.py | ||
|---|---|---|
| 5638 |
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) | |
| mercurial/commands.py | ||
|---|---|---|
| 5638 | 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. | |
Comment Actions
Kyle message made me realize that I had not sent mine, so I'm doing that now.
| mercurial/commands.py | ||
|---|---|---|
| 5638 |
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-*? | |
| mercurial/commands.py | ||
|---|---|---|
| 5638 | 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). | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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
| 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 | ||
| 5747–5750 | See my above comment about doing this later in the exchange when we have the final set of revisions. | |
Comment Actions
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.
Comment Actions
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)
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.