threads / patch / 46309

patchpush: add config option to --force-with-lease by default.

Subject: [PATCH] push: add config option to --force-with-lease by default.

## tl;dr

17 messages between Jul 3, 2017 and Jul 7, 2017. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Francesco Mazzoli· Jul 3, 2017, 21:18 UTC · lore

The flag can be overridden with `--no-force-with-lease`, or by passing the config via the command line.

Signed-off-by: Francesco Mazzoli <f@mazzo.li>
---
 Documentation/config.txt | 5 +++++
 builtin/push.c           | 3 +++
 cache.h                  | 1 +
 config.c                 | 4 ++++
 environment.c            | 1 +
 5 files changed, 14 insertions(+)
Show changes to 5 files +14 −0

Documentation/config.txt, builtin/push.c, cache.h, config.c, environment.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 06898a7..36fe882 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2537,6 +2537,11 @@ push.default::
 	specific workflows; for instance, in a purely central workflow
 	(i.e. the fetch source is equal to the push destination),
 	`upstream` is probably what you want.  Possible values are:
+
+push.alwaysforcewithlease::
+	When true, `--force-with-lease` is the default behavior when
+	using `push --force`. Explicit invocations of `--force-with-lease`
+	or `--no-force-with-lease` if present, take precedence.
 +
 --
 
diff --git a/builtin/push.c b/builtin/push.c
index 03846e8..96fc4b2 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -561,6 +561,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 
 	packet_trace_identity("push");
 	git_config(git_push_config, &flags);
+	if (push_always_force_with_lease) {
+		cas.use_tracking_for_rest = 1;
+	}
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	set_push_cert_flags(&flags, push_cert);
 
diff --git a/cache.h b/cache.h
index 96055c2..d31c972 100644
--- a/cache.h
+++ b/cache.h
@@ -830,6 +830,7 @@ enum push_default_type {
 extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
+extern int push_always_force_with_lease;
 
 enum object_creation_mode {
 	OBJECT_CREATION_USES_HARDLINKS = 0,
diff --git a/config.c b/config.c
index 1cd40a5..2a0dbe4 100644
--- a/config.c
+++ b/config.c
@@ -1317,6 +1317,10 @@ static int git_default_push_config(const char *var, const char *value)
 		}
 		return 0;
 	}
+	if (!strcmp(var, "push.alwaysforcewithlease")) {
+		push_always_force_with_lease = git_config_bool(var, value);
+		return 0;
+	}
 
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
diff --git a/environment.c b/environment.c
index d40b21f..79186c7 100644
--- a/environment.c
+++ b/environment.c
@@ -53,6 +53,7 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
+int push_always_force_with_lease = 0;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
-- 
2.7.4
Ævar Arnfjörð Bjarmason· Jul 3, 2017, 21:47 UTC · re: Francesco Mazzoli · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On Mon, Jul 03 2017, Francesco Mazzoli jotted:
> The flag can be overridden with `--no-force-with-lease`, or by
> passing the config via the command line.
Thanks for hacking on this. A couple of things:
* Most things (but not all) that configure `git whatevs --some-option`
  are configurable via whatevs.someOption, I think this should follow
  that convention. I.e. be push.forceWithLease not
  push.alwaysForceWithLease.
  See my
  https://public-inbox.org/git/20170324231013.23346-1-avarab@gmail.com/
  patch series for something that went through many of these cases
  (although I see I didn't send it all to list). Anyway, something like
  8/10 of our config variables for switches follow that
  convention. Let's use it for new config.
* It makes sense to also document this the git-push manpage. See
  e.g. how we document --follow-tags:
      Push all the refs that [...] This can also be specified with
      configuration variable push.followTags. For more information, see
      push.followTags in git-config(1).
   You should add something like that to --force-with-lease.
> +push.alwaysforcewithlease::
> +	When true, `--force-with-lease` is the default behavior when
> +	using `push --force`. Explicit invocations of `--force-with-lease`
> +	or `--no-force-with-lease` if present, take precedence.

Semantically this makes sense, and is exactly how most of these switch config variables work (and would be how it worked once refactored with my CLI arg config patch).

Francesco Mazzoli· Jul 3, 2017, 21:57 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On 3 July 2017 at 23:47, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 19 quoted lines
>
> On Mon, Jul 03 2017, Francesco Mazzoli jotted:
>
> > The flag can be overridden with `--no-force-with-lease`, or by
> > passing the config via the command line.
>
> Thanks for hacking on this. A couple of things:
>
> * Most things (but not all) that configure `git whatevs --some-option`
>   are configurable via whatevs.someOption, I think this should follow
>   that convention. I.e. be push.forceWithLease not
>   push.alwaysForceWithLease.
>
>   See my
>   https://public-inbox.org/git/20170324231013.23346-1-avarab@gmail.com/
>   patch series for something that went through many of these cases
>   (although I see I didn't send it all to list). Anyway, something like
>   8/10 of our config variables for switches follow that
>   convention. Let's use it for new config.

OK, my worry was about the fact that if I call it like the option I'd better support the full range of options available to `--force-with-lease`. Anyway, I'll change the name.

Show 8 quoted lines
> * It makes sense to also document this the git-push manpage. See
>   e.g. how we document --follow-tags:
>
>       Push all the refs that [...] This can also be specified with
>       configuration variable push.followTags. For more information, see
>       push.followTags in git-config(1).
>
>    You should add something like that to --force-with-lease.
OK, will do.
Francesco Mazzoli· Jul 3, 2017, 22:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

The flag can be overridden with `--no-force-with-lease`, or by passing the config via the command line.

Signed-off-by: Francesco Mazzoli <f@mazzo.li>
---
 Documentation/config.txt   | 5 +++++
 Documentation/git-push.txt | 4 +++-
 builtin/push.c             | 3 +++
 cache.h                    | 1 +
 config.c                   | 4 ++++
 environment.c              | 1 +
 6 files changed, 17 insertions(+), 1 deletion(-)
Show changes to 6 files +17 −1

Documentation/config.txt, Documentation/git-push.txt, builtin/push.c, cache.h, config.c, environment.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 06898a7..835b34b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2537,6 +2537,11 @@ push.default::
 	specific workflows; for instance, in a purely central workflow
 	(i.e. the fetch source is equal to the push destination),
 	`upstream` is probably what you want.  Possible values are:
+
+push.forceWithLease::
+	When true, `--force-with-lease` is the default behavior when
+	using `push --force`. Explicit invocations of `--force-with-lease`
+	or `--no-force-with-lease` take precedence.
 +
 --
 
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 0a63966..5182656 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -195,7 +195,9 @@ only if the "lease" is still valid.
 `--force-with-lease` alone, without specifying the details, will protect
 all remote refs that are going to be updated by requiring their
 current value to be the same as the remote-tracking branch we have
-for them.
+for them. This behavior can be made default using the `push.forceWithLease`
+configuration option. Consult linkgit:git-config[1]. For more
+information.
 +
 `--force-with-lease=<refname>`, without specifying the expected value, will
 protect the named ref (alone), if it is going to be updated, by
diff --git a/builtin/push.c b/builtin/push.c
index 03846e8..d8fa826 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -561,6 +561,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 
 	packet_trace_identity("push");
 	git_config(git_push_config, &flags);
+	if (push_force_with_lease) {
+		cas.use_tracking_for_rest = 1;
+	}
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	set_push_cert_flags(&flags, push_cert);
 
diff --git a/cache.h b/cache.h
index 96055c2..5d29e8e 100644
--- a/cache.h
+++ b/cache.h
@@ -830,6 +830,7 @@ enum push_default_type {
 extern enum branch_track git_branch_track;
 extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
+extern int push_force_with_lease;
 
 enum object_creation_mode {
 	OBJECT_CREATION_USES_HARDLINKS = 0,
diff --git a/config.c b/config.c
index 1cd40a5..f1b1e28 100644
--- a/config.c
+++ b/config.c
@@ -1317,6 +1317,10 @@ static int git_default_push_config(const char *var, const char *value)
 		}
 		return 0;
 	}
+	if (!strcmp(var, "push.forcewithlease")) {
+		push_force_with_lease = git_config_bool(var, value);
+		return 0;
+	}
 
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
diff --git a/environment.c b/environment.c
index d40b21f..fb36c1b 100644
--- a/environment.c
+++ b/environment.c
@@ -53,6 +53,7 @@ unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
 enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
+int push_force_with_lease = 0;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
-- 
2.7.4
Ævar Arnfjörð Bjarmason· Jul 3, 2017, 22:15 UTC · re: Francesco Mazzoli · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On Mon, Jul 03 2017, Francesco Mazzoli jotted:
A couple of things I didn't notice at first:
>  	git_config(git_push_config, &flags);
> +	if (push_always_force_with_lease) {
> +		cas.use_tracking_for_rest = 1;
> +	}
This should go in git_push_config.
> +	if (!strcmp(var, "push.alwaysforcewithlease")) {
> +		push_always_force_with_lease = git_config_bool(var, value);
> +		return 0;
> +	}

[As you noted on IRC] --force-with-lease takes args, but yours doesn't. Arguably this makes no sense whatsoever to have in the config, but something worth pointing out in the commit message.

We could make the config accept args, you'd call parse_push_cas_option(), but should we? I don't know.

Should this also apply to send-pack's --force-with-lease? Under the same option name or send-pack.forceWithLease? I also don't know...

Ævar Arnfjörð Bjarmason· Jul 3, 2017, 22:28 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On Mon, Jul 03 2017, Ævar Arnfjörð Bjarmason jotted:
> On Mon, Jul 03 2017, Francesco Mazzoli jotted:
>
> A couple of things I didn't notice at first:

Oh and also, makes sense to add tests for this, see t/t5533-push-cas.sh. I.e. just make sure the cases where the option works work with the config, and that the option overrides the config etc.

Show 21 quoted lines
>>  	git_config(git_push_config, &flags);
>> +	if (push_always_force_with_lease) {
>> +		cas.use_tracking_for_rest = 1;
>> +	}
>
> This should go in git_push_config.
>
>> +	if (!strcmp(var, "push.alwaysforcewithlease")) {
>> +		push_always_force_with_lease = git_config_bool(var, value);
>> +		return 0;
>> +	}
>
> [As you noted on IRC] --force-with-lease takes args, but yours
> doesn't. Arguably this makes no sense whatsoever to have in the config,
> but something worth pointing out in the commit message.
>
> We could make the config accept args, you'd call
> parse_push_cas_option(), but should we? I don't know.
>
> Should this also apply to send-pack's --force-with-lease? Under the same
> option name or send-pack.forceWithLease? I also don't know...
Junio C Hamano· Jul 4, 2017, 17:51 UTC · re: Francesco Mazzoli · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

Francesco Mazzoli <f@mazzo.li> writes:
Show 27 quoted lines
> The flag can be overridden with `--no-force-with-lease`, or by
> passing the config via the command line.
>
> Signed-off-by: Francesco Mazzoli <f@mazzo.li>
> ---
>  Documentation/config.txt | 5 +++++
>  builtin/push.c           | 3 +++
>  cache.h                  | 1 +
>  config.c                 | 4 ++++
>  environment.c            | 1 +
>  5 files changed, 14 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 06898a7..36fe882 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2537,6 +2537,11 @@ push.default::
>  	specific workflows; for instance, in a purely central workflow
>  	(i.e. the fetch source is equal to the push destination),
>  	`upstream` is probably what you want.  Possible values are:
> +
> +push.alwaysforcewithlease::
> +	When true, `--force-with-lease` is the default behavior when
> +	using `push --force`. Explicit invocations of `--force-with-lease`
> +	or `--no-force-with-lease` if present, take precedence.
>  +
>  --
I suspect this may be going in a wrong direction.  

People have been burned by the lazy "--force-with-lease" that does not say what object to expect there and forces the command to DWIM incorrectly what the remote's ref ought to be pointing at. This change encourages its use without the user being painfully aware of that danger. Whenever you say "push --force", you'd be using the dangerous "--force-with-lease" that does not specify what the expected current state of the remote is. The end result gives an illusion of being safer than a simple "--force", without being not really safer.

I'd understand more if there were two new (and orthogonal) options, though:

 - disable the use of "--force" option, telling the user to use
   "--force-with-lease=<object>" instead.
 - disable the DWIM based on the remote-tracking branches when
   "--force-with-lease[=<refname>]" is used, i.e. error out when the
   option is used without a specific object to expect.
Francesco Mazzoli· Jul 5, 2017, 06:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On 4 July 2017 at 19:51, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> People have been burned by the lazy "--force-with-lease" that does
> not say what object to expect there and forces the command to DWIM
> incorrectly what the remote's ref ought to be pointing at.  This
> change encourages its use without the user being painfully aware of
> that danger.  Whenever you say "push --force", you'd be using the
> dangerous "--force-with-lease" that does not specify what the
> expected current state of the remote is.  The end result gives an
> illusion of being safer than a simple "--force", without being
> not really safer.

Could you clarify the danger you're referring to? E.g. give an example of surprising --force-with-lease behavior that we do not want to encourage?

Thanks, Francesco

Junio C Hamano· Jul 5, 2017, 07:43 UTC · re: Francesco Mazzoli · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli <f@mazzo.li> wrote:
>
> Could you clarify the danger you're referring to? E.g. give an example
> of surprising --force-with-lease behavior that we do not want to
> encourage?
https://public-inbox.org/git/1491617750.2149.10.camel@mattmccutchen.net/
Francesco Mazzoli· Jul 5, 2017, 08:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On 5 July 2017 at 09:43, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli <f@mazzo.li> wrote:
>>
>> Could you clarify the danger you're referring to? E.g. give an example
>> of surprising --force-with-lease behavior that we do not want to
>> encourage?
>
> https://public-inbox.org/git/1491617750.2149.10.camel@mattmccutchen.net/

Thanks for clarifying, I had not encountered this because of my git workflow. I'd also be happy with a config item that simply disables `--force` and suggests `--force-with-lease`, but then we'd need a flag to override that config item when the user definitely wants to force push.

So we would have something like
* `push.disableForce`: config flag that disables `--force` and suggests
    `--force-with-lease` instead;
* `--disable-force` and `--no-disable-force`, config flags to tune the above
    config parameter at will.
What do you think?

Thanks, Francesco

Junio C Hamano· Jul 5, 2017, 15:17 UTC · re: Francesco Mazzoli · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

Francesco Mazzoli <f@mazzo.li> writes:
Show 8 quoted lines
> So we would have something like
>
> * `push.disableForce`: config flag that disables `--force` and suggests
>     `--force-with-lease` instead;
> * `--disable-force` and `--no-disable-force`, config flags to tune the above
>     config parameter at will.
>
> What do you think?

The take-away lesson that the earlier thread gave me was that the order in which the three options are ranked by their desirebility in the UI (and the order we would like to encourage users to use) is, from the most to the least preferrable:

 - "--force-with-lease=<ref>:<expect>" that is safer than "--force";
 - "--force" that is known to be dangerous, and does not pretend to
   be anything but;
 - "--force-with-lease" that pretends to be safer but is not.

The last form should eventually be eliminated, as there is no way to correctly intuit what the expected object should be.

To me, a disableForce configuration that encourages use of either the best one or the worst one alone does not look like a step forward, unless we also have a change to disable the last form.

Francesco Mazzoli· Jul 5, 2017, 16:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

Show 16 quoted lines
> On 5 Jul 2017, at 17:17, Junio C Hamano <gitster@pobox.com> wrote:
> 
> The take-away lesson that the earlier thread gave me was that the
> order in which the three options are ranked by their desirebility
> in the UI (and the order we would like to encourage users to use)
> is, from the most to the least preferrable:
> 
> - "--force-with-lease=<ref>:<expect>" that is safer than "--force";
> 
> - "--force" that is known to be dangerous, and does not pretend to
>   be anything but;
> 
> - "--force-with-lease" that pretends to be safer but is not.
> 
> The last form should eventually be eliminated, as there is no way to
> correctly intuit what the expected object should be.

What's not clear to me is what the intended workflow using `--force-with-lease=<ref>:<expect>` is. Intuitively it seems extremely cumbersome to manually pluck a revision each time, especially when dealing with commits that all have the same description.

On the other hand for my workflow `--force-with-lease` works quite well because I tend to use it in cases where me and a colleague are working on the same PR, and thus I'm not doing anything else (including fetching).

Moreover, it seems to me that the problem `--force-with-lease` is just one of marketing. `--force-with-lease` is strictly more "safe" than `--force` in the sense that it'll reject some pushes that `--force` will let through. I think that if we advertise it better including its drawbacks it can still be better than no checks at all.

Francesco
Mike Rappazzo· Jul 5, 2017, 18:51 UTC · re: Francesco Mazzoli · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On Wed, Jul 5, 2017 at 12:43 PM, Francesco Mazzoli <f@mazzo.li> wrote:
Show 33 quoted lines
>> On 5 Jul 2017, at 17:17, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> The take-away lesson that the earlier thread gave me was that the
>> order in which the three options are ranked by their desirebility
>> in the UI (and the order we would like to encourage users to use)
>> is, from the most to the least preferrable:
>>
>> - "--force-with-lease=<ref>:<expect>" that is safer than "--force";
>>
>> - "--force" that is known to be dangerous, and does not pretend to
>>   be anything but;
>>
>> - "--force-with-lease" that pretends to be safer but is not.
>>
>> The last form should eventually be eliminated, as there is no way to
>> correctly intuit what the expected object should be.
>
> What's not clear to me is what the intended workflow using
> `--force-with-lease=<ref>:<expect>` is. Intuitively it seems extremely
> cumbersome to manually pluck a revision each time, especially when
> dealing with commits that all have the same description.
>
> On the other hand for my workflow `--force-with-lease` works quite well
> because I tend to use it in cases where me and a colleague are working
> on the same PR, and thus I'm not doing anything else (including fetching).
>
> Moreover, it seems to me that the problem `--force-with-lease` is
> just one of marketing. `--force-with-lease` is strictly more "safe"
> than `--force` in the sense that it'll reject some pushes that `--force`
> will let through. I think that if we advertise it better including its
> drawbacks it can still be better than no checks at all.
>
> Francesco

I am in your camp on this, and I will also only ever explicitly fetch. I would hate for --force-with-lease to disappear.

However, I believe that the problem is that there are many third party tools which do a fetch behind the scenes (for example Atlassian SourceTree). This can update the local refs without a user necessarily thinking about it. This can lead to a force-with-lease being used unsafely (without the stated lease).

_Mike
Junio C Hamano· Jul 6, 2017, 19:13 UTC · re: Francesco Mazzoli · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

Francesco Mazzoli <f@mazzo.li> writes:
> Moreover, it seems to me that the problem `--force-with-lease` is
> just one of marketing. `--force-with-lease` is strictly more "safe"
> than `--force` in the sense that it'll reject some pushes that `--force`
> will let through.

By that logic, a hypothetical update to `--force` that makes 1/3 of the attempted forced push randomly would make it safer than the current `--force`, wouldn't it?

When third-party tools fetch and update remote-tracking branches behind the users' back, the safety based on the stability of remote-tracking branches are defeated. And the biggest problem is that the way `--force-with-lease` misbehaves---it is not like it randomly and mistakenly stops the push that could go through; it lets through what shouldn't.

See the other patch I sent just now---with something like that patch that lets those like you, who know their remote-tracking branches are reliable, use the lazy form, while disabling it by default for others (until they examine their situation and perhaps disable the problematic auto-fetching) in place, I do not think it is a bad idea to advertise --force-with-lease a safer option than --force (because those for whom it is not safer will not be able to use it).

Francesco Mazzoli· Jul 7, 2017, 06:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On 6 July 2017 at 21:13, Junio C Hamano <gitster@pobox.com> wrote:
> By that logic, a hypothetical update to `--force` that makes 1/3 of
> the attempted forced push randomly would make it safer than the
> current `--force`, wouldn't it?

It would. However, this additional safety is not really meaningful to any workflow, while the one added by `--force-with-lease` is.

Show 14 quoted lines
> When third-party tools fetch and update remote-tracking branches
> behind the users' back, the safety based on the stability of
> remote-tracking branches are defeated.  And the biggest problem
> is that the way `--force-with-lease` misbehaves---it is not like
> it randomly and mistakenly stops the push that could go through;
> it lets through what shouldn't.
>
> See the other patch I sent just now---with something like that patch
> that lets those like you, who know their remote-tracking branches
> are reliable, use the lazy form, while disabling it by default for
> others (until they examine their situation and perhaps disable the
> problematic auto-fetching) in place, I do not think it is a bad idea
> to advertise --force-with-lease a safer option than --force (because
> those for whom it is not safer will not be able to use it).

Fair enough, I'm OK with enabling it with some config. I'd still like a way to enable it by default if I want though.

Thanks, Francesco

Ævar Arnfjörð Bjarmason· Jul 5, 2017, 11:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On Wed, Jul 05 2017, Junio C. Hamano jotted:
Show 7 quoted lines
> On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli <f@mazzo.li> wrote:
>>
>> Could you clarify the danger you're referring to? E.g. give an example
>> of surprising --force-with-lease behavior that we do not want to
>> encourage?
>
> https://public-inbox.org/git/1491617750.2149.10.camel@mattmccutchen.net/

In the context of this patch I don't understand why you're concerned that making --force mean --force-with-lease makes things worse.

See my https://public-inbox.org/git/CACBZZX48RanjHsv1UsnxkbxRtqKRGgMcgmtVqQmR84H5j8awqQ@mail.gmail.com/ (follow-up to the E-Mail you posted):

    To me the *main* feature of --force-with-lease is that it's less
    shitty than --force, without imposing too much UI overhead. We have to
    be really careful not to make --force-with-lease so complex by default
    that people just give u and go back to using --force, which would be
    worse than either whatever current problems there are with the
    current --force-with-lease behavior, or anything we replace it with.

I.e. yes there are workflows with some background auto-update that will make it less safe, which I documented in f17d642d3b ("push: document & test --force-with-lease with multiple remotes", 2017-04-19).

But it is still the case that --force-with-lease is categorically a more safer option than simply --force, which has none of the safety --force-with-lease has. It would still wipe away history in this scenario you're pointing out *and others*.

Surely the point of having an option like this is to have a net reduction in complexity.

I think it can be argued that it's bad UI design though to have --force mean different things depending on the config, and we'd be better off with a patch that disables --force.

Ævar Arnfjörð Bjarmason· Jul 5, 2017, 15:48 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] push: add config option to --force-with-lease by default.

On Wed, Jul 5, 2017 at 1:26 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 36 quoted lines
>
> On Wed, Jul 05 2017, Junio C. Hamano jotted:
>
>> On Tue, Jul 4, 2017 at 11:34 PM, Francesco Mazzoli <f@mazzo.li> wrote:
>>>
>>> Could you clarify the danger you're referring to? E.g. give an example
>>> of surprising --force-with-lease behavior that we do not want to
>>> encourage?
>>
>> https://public-inbox.org/git/1491617750.2149.10.camel@mattmccutchen.net/
>
> In the context of this patch I don't understand why you're concerned
> that making --force mean --force-with-lease makes things worse.
>
> See my
> https://public-inbox.org/git/CACBZZX48RanjHsv1UsnxkbxRtqKRGgMcgmtVqQmR84H5j8awqQ@mail.gmail.com/
> (follow-up to the E-Mail you posted):
>
>     To me the *main* feature of --force-with-lease is that it's less
>     shitty than --force, without imposing too much UI overhead. We have to
>     be really careful not to make --force-with-lease so complex by default
>     that people just give u and go back to using --force, which would be
>     worse than either whatever current problems there are with the
>     current --force-with-lease behavior, or anything we replace it with.
>
> I.e. yes there are workflows with some background auto-update that will
> make it less safe, which I documented in f17d642d3b ("push: document &
> test --force-with-lease with multiple remotes", 2017-04-19).
>
> But it is still the case that --force-with-lease is categorically a more
> safer option than simply --force, which has none of the safety
> --force-with-lease has. It would still wipe away history in this
> scenario you're pointing out *and others*.
>
> Surely the point of having an option like this is to have a net
> reduction in complexity.
I mean reduction in risk...
> I think it can be argued that it's bad UI design though to have --force
> mean different things depending on the config, and we'd be better off
> with a patch that disables --force.

← back to recent threads