Re: [PATCH] push: disable lazy --force-with-lease by default

From: Stefan Beller <sbeller@google.com>
Date: Jul 6, 2017, 19:38 UTC
Message-ID: <CAGZ79kbCo3EjRGR_iE=6CJ2ou=4gMv3=t+s1ctGhUbZg8UDixA@mail.gmail.com>
In-Reply-To: <xmqq37a9fl8a.fsf_-_@gitster.mtv.corp.google.com>

On Thu, Jul 6, 2017 at 11:56 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 66 quoted lines

> "git push --force-with-lease=<branch>:<expect>" makes sure that
> there is no unexpected changes to the branch at the remote while you
> prepare a rewrite based on the old state of the branch.  This
> feature came with an experimental option that allows :<expect> part
> to be omitted by using the tip of remote-tracking branch that
> corresponds to the <branch>.
>
> It turns out that some people use third-party tools that fetch from
> remote and update the remote-tracking branches behind users' back,
> defeating the safety relying on the stability of the remote-tracking
> branches.  We have some warning text that was meant to be scary
> sounding in our documentation, but nevertheless people seem to be
> bitten.  cf. https://public-inbox.org/git/1491617750.2149.10.camel@mattmccutchen.net/
> for a recent example.
>
> Let's disable the form that relies on the stability of remote-tracking
> branches by default, and allow users who _know_ their remote-tracking
> branches are stable to enable it with a configuration variable.
>
> This problem was predicted from the very beginning; see 28f5d176
> (remote.c: add command line option parser for "--force-with-lease",
> 2013-07-08).
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * This is a bit overdue safety fix that we should have done long
>    time ago.  If we had this, I do not think it makes it riskier to
>    forbid --force and tell people to use --force-with-lease.
>
>  Documentation/config.txt   |  5 +++++
>  Documentation/git-push.txt |  5 +++--
>  builtin/send-pack.c        |  5 +++++
>  remote.c                   | 16 ++++++++++++----
>  remote.h                   |  2 ++
>  send-pack.c                |  1 +
>  t/t5533-push-cas.sh        | 19 +++++++++++++++++--
>  transport-helper.c         |  5 +++++
>  transport.c                |  5 +++++
>  9 files changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 06898a7498..2f929315a2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2588,6 +2588,11 @@ new default).
>
>  --
>
> +push.allowLazyForceWithLease::
> +       If set to true, allow the `--force-with-lease` option
> +       without the expected object name (i.e. expecting the objects
> +       at the tip of corresponding remote-tracking branches).
> +
>  push.followTags::
>         If set to true enable `--follow-tags` option by default.  You
>         may override this configuration at time of push by specifying
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 0a639664fd..1fa01210a2 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -212,8 +212,9 @@ must not already exist.
>  +
>  Note that all forms other than `--force-with-lease=<refname>:<expect>`
>  that specifies the expected current value of the ref explicitly are
> -still experimental and their semantics may change as we gain experience

This indicates that this feature is not 'experimental' any more, but disabled (for safety reasons as described below). This implies we will not change the heuristic for push.allowLazyForceWithLease easily.

Upon reading this documentation and the safety issue, I thought
one could have used the reflog to make it safer as well:
 "I have (manually) inspected the remote tracking branch
  just before lunch, so now I can safely push with lease after lunch"

would translate to e.g. "--force-with-lease-safe--inspection-time=1h", which would make sure that no reflog entries for the given branches exist in the last hour.

Just as food for thought.

Show 6 quoted lines

>  test_expect_success 'push to update (protected)' '
> @@ -78,7 +79,6 @@ test_expect_success 'push to update (protected, tracking, forced)' '
>         (
>                 cd dst &&
>                 test_commit E &&
> -               git ls-remote . refs/remotes/origin/master >expect &&

This seems unrelated, so ideally it is a separate commit? Fine with me though.

Show 5 quoted lines

> @@ -91,6 +91,7 @@ test_expect_success 'push to update (allowed)' '
>         (
>                 cd dst &&
>                 test_commit D &&
> +               git config push.allowLazyForceWithLease false &&

Here I thought

    test_config -C dst ...

at the beginning might be useful, though ..

Show 8 quoted lines

> @@ -103,6 +104,10 @@ test_expect_success 'push to update (allowed, tracking)' '
>         (
>                 cd dst &&
>                 test_commit D &&
> +               git config push.allowLazyForceWithLease false &&
> +               test_must_fail git push --force-with-lease=master origin master 2>err &&
> +               grep "lazy force-with-lease" err &&
> +               git config --unset push.allowLazyForceWithLease &&

.. here the -C is not useful, so just using it once above may not be a good idea. For more dense and readable tests (also faster?), have you considered using passing the option via -c instead of setting and unsetting it?

Thanks, Stefan