Re: [PATCH v9 1/3] push: add reflog check for "--force-if-includes"

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 2 Oct 2020, Johannes Schindelin wrote:

>
> On Thu, 1 Oct 2020, Srinidhi Kaushik wrote:
>
> > Add a check to verify if the remote-tracking ref of the local branch
> > is reachable from one of its "reflog" entries.
> >
> > The check iterates through the local ref's reflog to see if there
> > is an entry for the remote-tracking ref and collecting any commits
> > that are seen, into a list; the iteration stops if an entry in the
> > reflog matches the remote ref or if the entry timestamp is older
> > the latest entry of the remote ref's "reflog". If there wasn't an
> > entry found for the remote ref, "in_merge_bases_many()" is called
> > to check if it is reachable from the list of collected commits.
> >
> > When a local branch that is based on a remote ref, has been rewound
> > and is to be force pushed on the remote, "--force-if-includes" runs
> > a check that ensures any updates to the remote-tracking ref that may
> > have happened (by push from another repository) in-between the time
> > of the last update to the local branch (via "git-pull", for instance)
> > and right before the time of push, have been integrated locally
> > before allowing a forced update.
> >
> > If the new option is passed without specifying "--force-with-lease",
> > or specified along with "--force-with-lease=<refname>:<expect>" it
> > is a "no-op".
> >
> > Calls to "in_merge_bases_many()" return different results depending
> > on whether the "commit-graph" feature is enabled or not -- it is
> > temporarily disabled when the check runs [1].
> >
> > [1] https://lore.kernel.org/git/xmqqtuvhn6yx.fsf@gitster.c.googlers.com
>
> I can verify that the multiple calls to `in_merge_bases_many()` lead to a
> problem, and I intend to debug this further, but it is the wrong function
> to call to begin with.

It was actually Stolee who figured this out: the shortcut at the beginning
of `in_merge_bases_many()` which tries to exit early when the generation
number of the `commit` indicates that it cannot be reached from the
`reference` commits (because their generation numbers are smaller) has a
bug in the logic. Obviously, the generation numbers are only used when
commit-graph is used, therefore things broke only in the `linux-gcc` job.

Stolee will send out a patch shortly.

Having said that, the change I suggested (to use `get_reachable_subset()`
instead of repeated `in_merge_bases_many()`) is _still_ the right thing to
do: we are not actually interested in the merge bases at all, but in
reachability, and in the future there might be more efficient ways to
determine that than painting down all the way to merge bases.

Thanks,
Dscho