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

From: Junio C Hamano <[email-masked]>
Date: Sat, 19 Sep 2020 10:48:00 -0700

Johannes Schindelin <[email-masked]> writes:

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