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

> Hi Srinidhi
> 
> I think this is moving forward in the right direction, I've got a couple of
> comments below. Note I've only looked at the first part of the patch as I'm
> not that familiar with the rest of the code.
> 
> On 23/09/2020 08:30, 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.
> > 
> > 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 remote-tracking refs that may have
> 
> nit pick - there is only one remote tracking ref for each local ref

> > last checkout,
>
> more generally it is the last time we updated the local branch to
> incorporate any fetched changes in the remote tracking branch, this includes
> `pull --rebase` `pull --merge` as well as checking out the remote ref
> 
> > and right before the time of push, have been integrated
> > locally before allowing a forced updated.
> > 
> > A new field "use_force_if_includes" has been added to "push_cas_option",
> > which is set to "1" when "--force-if-includes" is specified as an
> > option in the command line or as a configuration setting.
> > 
> > The struct "ref" has two new bit-fields:
> >    - check_reachable:
> >      Set when we have to run the new check on the ref, and the remote
> >      ref was marked as "use_tracking" or "use_tracking_for_rest" by
> >      compare-and-swap (if the "the remote tip must be at the expected
> >      commit" condition is not specified); "apply_push_cas()" has been
> >      updated to check if this field is set and run the check.
> > 
> >    - unreachable:
> >      Set if the ref is unreachable from any of the "reflog" entries of
> >      its local counterpart.
> > 
> > "REF_STATUS_REJECT_REMOTE_UPDATED" has been added to the "status"
> > enum to imply that the ref failed the check; "case" statements in
> > "send-pack", "transport" and "transport-helper" have been updated
> > accordingly to catch this status when set.
> 
> This is quite a long description of the implementation, I think it would be
> more helpful to the reader to concentrate on what the new feature is and why
> it is useful.

> > [...]
> It's great that you've incorporated a date check, however I think we need to
> check the local reflog timestamp against the last time the remote ref was
> updated (i.e. the remote reflog timestamp), not the commit date of the
> commit that the remote ref points too. We are interested in whether the
> local branch has incorporated the remote branch since the last time the
> remote branch was updated.

> > +		return -1;
> > +
> > +	/* An entry was found. */
> > +	if (oideq(n_oid, &cb->remote_commit->object.oid))
> > +		return 1;
> > +
> > +	/* Lookup the commit and append it to the list. */
> > +	if ((commit = lookup_commit_reference(the_repository, n_oid)))
> > +		add_commit(cb->local_commits, commit);
> 
> I think Junio suggested batching the commits for the merge base check into
> small groups, rather than checking them all at once