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

[Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>]

Hi Phillip,

On 09/23/2020 11:18, Phillip Wood wrote:
> 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

You're right, I will correct that. 
> > happened (by push from another repository) in-between the time of the
> 
> s/push/fetch/ ?

Well, I was hoping it implies that there was a push made by another
person to the ref while a rewrite is happening locally. 
 
> > 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.

OK, I will try to minimize the description a bit further.
 
> > 
> > When "--force-is-includes" is used along with "--force-with-lease",
> 
> s/-is-/-if-/

Oh, this happened again. Sorry!

> > the check runs only for refs marked as "if_includes". If the option
> > is passed without specifying "--force-with-lease", or specified along
> > with "--force-with-lease=<refname>:<expect>" it is a "no-op".

There's another typo: s/if_includes/check_reachable/.
 
> If I've understood this correctly `--force-if-includes` does nothing on its
> own - I had hoped it would imply --force-with-lease

Yes, it does nothing if not used with  "--force-with-lease", or when
used with it and the exact commit to be expected is specified for
"--force-with-lease" (i.e., with --force-with-lease=<ref>:<expect>).

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

Got it. I misread the requirement here; will update.

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

Doesn't stopping the iteration early with the date check collect fewer
commits? Would batching still be necessary?

Thanks.
-- 
Srinidhi Kaushik