Re: [PATCH] push: make `--force-with-lease[=<ref>]` safer

[Junio C Hamano <gitster@pobox.com>]

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:

> The `--force-with-lease` option in `git-push`, makes sure that
> refs on remote aren't clobbered by unexpected changes when the
> "<expect>" ref value is explicitly specified.
>
> For other cases (i.e., `--force-with-lease[=<ref>]`) where the tip
> of the remote tracking branch is populated as the "<expect>" value,
> there is a possibility of allowing unwanted overwrites on the remote
> side when some tools that implicitly fetch remote-tracking refs in
> the background are used with the repository. If a remote-tracking ref
> was updated when a rewrite is happening locally and if those changes
> are pushed by omitting the "<expect>" value in `--force-with-lease`,
> any new changes from the updated tip will be lost locally and will
> be overwritten on the remote.

Hmph, I am not sure if we are on the same page as the problem with
the form of force-with-lease without <expect>.

In this sequence of end-user operation

    $ git checkout --detach origin/target
    ... edit working tree files ...
    $ git commit --amend -a
    $ git push origin +HEAD:target

the user wanted to fix the topmost commit at the 'target' branch at
the origin repository, and force-update it.

The --force-with-lease is a way to make sure that the only commit
being lost by the force-update is the commit the user wanted to
amend.  If other users pushed to the 'target' branch in the
meantime, the forced push at the last step will lose it.

    $ git checkout --detach origin/target
    $ TO_LOSE=$(git rev-parse HEAD)
    ... edit working tree files ...
    $ git commit --amend -a
    $ git push origin --force-with-lease=target:$TO_LOSE HEAD:target

So we say "I knew, when I started working on the replacement, I
started at the commit $TO_LOSE; please stop my forced push if the
tip of 'target' was moved by somebody else, away from $TO_LOSE".

The force-with-lease without the exact <expect> object name, i.e.

    $ git push origin --force-with-lease=target HEAD:target

would break if 'origin/target' was updated anytime between the time
when the first "git checkout --detach" step finishes and the time
the last "git push" is run, because 'origin/target' would be
different from $TO_LOSE and things that were pushed to 'origin/target'
by others in the meantime will be lost, in addition to $TO_LOSE commit
that the user is willing to discard and replace.

> This problem can be addressed by checking the `reflog` of the branch
> that is being pushed and verify if there in a entry with the remote
> tracking ref.

Sorry, but it is unclear how reflog would help.

Before the "git checkout" step in the example, there would have been
a "git fetch" from the origin that brought the remote-tracking
branch 'origin/target' to the current state with a reflog entry for
it.  If an automated background process makes another fetch while
the user is editing files in the working tree, such a fetch may also
add another reflog entry for that action.

Unless you make a snapshot of the reflog state immediately after you
do "git checkout" in the example, you wouldn't know if there were
unexpected updates to the remote-tracking branch even if you check
the reflog.

Besides, you do not control the parenthood relationship between the
commits _other_ people push and update to 'target' branch at the
'origin' repository, so you cannot rely on the topology among them
to make any decision.  Other people may be force pushing to the
branch while you are preparing the commit to replace $TO_LOSE by
force pushing.

> +	The check ensures that the commit at the tip of the remote-tracking
> +	branch -- which may have been implicitly updated by tools that fetch
> +	remote refs by running linkgit:git-fetch[1] in the background -- has
> +	been integrated locally, when holding the "lease".

The problem with "expect-less" form is that it does not hold the
lease at all.  The point of "hold the lease" by giving an explicit
$TO_LOSE is to force a push that does not fast-forward, so if you
iterate over the remote-tracking branch at the time of "push", if
you find more commits built on top of $TO_LOSE because a background
fetch updated the branch, or if you find NO commits on top of $TO_LOSE
because no background fetch happened in the meantime, what would be
pushed would not fast-forward.  So I am not sure what the point of
iterating over reflog.

If we want an option to force a safer behaviour, I think adding a
configuration variable to forbid the form of force-with-lease
without <expect> would be one way to help.  Perhaps make it the
default, while allowing those who want to live dangerously to
explicitly turn it off.

Thanks.