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

[Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>]

Hi Junio,
Thanks for taking the time to review this patch.

On 09/07/2020 09:14, Junio C Hamano wrote:
>> 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.

Sorry, that commit message should have been worded in a better way.
What I originally meant to say was: losing any _new_ changes that
were made on the remote during a rewrite, i.e., when we have a starting
point ($TO_LOSE) from the remote-tracking ref that we want to base our
rewrite on (after checkout) and then if the remote-tracking ref gets
updated by a push from another user, _and_ we don't use an "<expect>"
for `--force-with-lease`, there is a possibility that changes from the
other push would be lost because we're basing our rewrite on an older
version (?) of the remote-tracking ref, and our push would overwrite
its new changes because the value "<expect>" when omitted would point
to the updated tip of the remote instead. The condition:

	if (!oideq(&ref->old_oid, &ref->old_oid_expect))

would evaluate false since we're using `use_tracking`. This essentially
reduces the behavior of `--force-with-lease=<ref>` to `--force` in this
scenario.

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

Right, I agree with what is described above. But, in this patch, we are
looking at the reflog of the _local_ branch that is going to be updated
on the remote side. The point of going through the reflog is to see if
the current tip its remote-tracking branch is present in one of the
reflog entries implying that any new changes (pushes from another user)
in the meantime aren't ignored and overwritten with our push.
Would that be an incorrect assumption?

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

Yes, that sounds like a good way to mitigate this issue; but that being
said, setups where `--force-with-lease` is being used as an alias for
`--force` should probably be taken into consideration though.

Thanks.
--
Srinidhi Kaushik