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

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