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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2020, 17:48 UTC
Message-ID: <xmqqsgbdk69b.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2009161214030.56@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 22 quoted lines

> No, I don't think that would suffice. For several reasons:
>
> - the user could have checked out the remote-tracking branch directly
>   (`git switch --detach origin/my-branch`), as opposed to actually
>   integrating the revision into the branch at some stage. I know that I do
>   that sometimes.
>
> - even if the remote-tracking branch has not been checked out directly, it
>   might have been `git pull --rebase`d into a _different_ branch, in which
>   case the reflog of `HEAD` would say "yep, I saw this commit!" but that
>   would not mean that it was integrated into the branch we want to
>   (force-)push.
>
> - the reflog of the `HEAD` is worktree-specific, and if the user pushes
>   from a different worktree (e.g. to push multiple branches at the same
>   time, or because the push is scripted and run from a different working
>   directory), it would be missed.
>
> - and if we take a little step back, we do see that the reflog of `HEAD`
>   _does_ answer a different question from what we actually asked.
>
> Sure, it would be faster, but is that worth the consequences?

It is not about "would be faster" but is trying to be more inclusive of different workflows.

One minor downside consequence of not looking at the reflog of HEAD is that it does not make the new option useful for those who could benefit from it.

They detach HEAD at the tip of the remote-tracking branch, work on the replacement history while on the detached HEAD, and then force update the local branch from there once they are done (this is a variant to the first one you said you "do sometimes"). This is a handy technique to allow them to always compare the previous round of their effort (which is untouched while they prepare the new iteration on detached HEAD) with what they achieved so far on HEAD.

After finishing, they would go on to deal with other branches, so the local branch prepared to be force-pushed may no longer be the current one. Without checking with the reflog of HEAD, there is no way to notice that the local branch to be force-pushed (which no longer is the current branch) was built using the commit at the tip of the remote-tracking branch we are losing, because the branch's reflog would have only one entry that records where it was before adjusting for the updated remote-tracking branch, and where it is now after rebuilding.

It is inevitable for a heuristics like the one that was proposed to work only for those with some workflows while excluding those with some other workflows, and that is why I said it is "minor" downside that it does not make the feature useful for some people who could potentially benefit and also that is why I think it is perfectly OK to draw the line there and not support them. After all, the line has to be drawn somewhere, and the looser the heuristics are, the wider the potential audience *and* at the same time the wider the potential of false positives.

Where we drew the line and its consequences must be communicated clearly in the documentation, though.

Thanks.