Phillip Wood <phillip.wood123@xxxxxxxxx> 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: >> 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. > >> + 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.
