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
