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

Phillip Wood [off-list ref] writes:

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:

quoted

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

Yup, "updates to the remote-tracking ref".

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.

Thanks for pointing it out.

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.

Yes, exactly.

quoted

+		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

That was before you suggested to cut the reflog traversal using the
reflog timestamp.  The code in earlier rounds of this series wanted
to read the reflog entries all the way down to the beginning of time,
and it made sense to batch in order to limit the traversal.

Now we found out that using only the local reflog entries more
recent than the latest update to the remote-tracking branch, and
because these local reflog entries will already be read in the above
code anyway, I do not see a point in batching.

Even if we were to batch calls to git_merge_bases_many(), it won't
happen in the quoted part of the code, where it first tries to see
any of the recent local reflog entries directly point at the object
with oideq().  While it is doing so, the only thing it does for the
later merge-base phase is to collect them in local_commits[] array.
Chunking can, if desired, be done by carving the array into pieces
and calling git_merge_bases_many() in is_reachable_in_reflog()
function.

Thanks for a careful review.