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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 16, 2020, 10:20 UTC
Message-ID: <nycvar.QRO.7.76.6.2009161214030.56@tvgsbejvaqbjf.bet>
In-Reply-To: <20200916053141.GA29901@mail.clickyotomy.dev>

Hi,

On Wed, 16 Sep 2020, Srinidhi Kaushik wrote:

Show 23 quoted lines

> On 09/14/2020 13:08, Junio C Hamano wrote:
> >>> [...]
> >>>
> >>> Good catch. Would adding in_merge_bases() along with checking if OIDs
> >>> are equal for each reflog entry in oid_in_reflog_ent() address the
> >>> problem? That way, we would check if remote ref is reachable from
> >>> one of the entries?
> >
> > That sounds very expensive.
>
> Yes, you're right about that.
>
> > If we switched to check the reflog of HEAD (and not any particular
> > local branch's reflog), then "has this ever been checked out", tests
> > would suffice, no?  We detach at the tip of the remote-tracking
> > branch and then reapply our work one by one in such a case, so we
> > should have the record of having their tip in the working tree at
> > some point (i.e. at the beginning of the "rebase" phase of the "pull
> > --rebase" operation).
>
> Interesting, I think that might work! Since HEAD moves around and
> records all the movements, if the remote was ever checked out there
> should be an entry in the reflog.

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?

I really think we need to stick to looking at the reflog of the asked-for branch. And to make that faster, we should have a first loop using `oideq()` and failing that check, run a second loop using `is_in_merge_bases()`.

Ciao, Dscho