From: Johannes Schindelin Date: Wed, 16 Sep 2020 12:35:04 GMT Subject: Re: [PATCH v3 1/7] remote: add reflog check for "force-if-includes" Message-ID: In-Reply-To: <20200913145413.18351-2-shrinidhi.kaushik@gmail.com> Hi Srinidhi, On Sun, 13 Sep 2020, Srinidhi Kaushik wrote: > diff --git a/remote.c b/remote.c > index 420150837b..e4b2d85a6f 100644 > --- a/remote.c > +++ b/remote.c > @@ -1484,6 +1484,36 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, > force_ref_update = 1; > } > > + /* > + * If the tip of the remote-tracking ref is unreachable > + * from any reflog entry of its local ref indicating a > + * possible update since checkout; reject the push. > + * > + * There is no need to check for reachability, if the > + * ref is marked for deletion. > + */ > + if (ref->if_includes && !ref->deletion) { > + /* > + * If `force_ref_update' was previously set by > + * "compare-and-swap", and we have to run this > + * check, reset it back to the original value > + * and update it depending on the status of this > + * check. > + */ > + force_ref_update = ref->force || force_update; > + > + if (ref->unreachable) > + reject_reason = > + REF_STATUS_REJECT_REMOTE_UPDATED; > + else > + /* > + * If updates from the remote-tracking ref > + * have been integrated locally; force the > + * update. > + */ > + force_ref_update = 1; > + } > + > /* > * If the update isn't already rejected then check > * the usual "must fast-forward" rules. > @@ -2272,11 +2302,74 @@ static int remote_tracking(struct remote *remote, const char *refname, > return 0; > } > > +static int ref_reachable(struct object_id *o_oid, struct object_id *n_oid, > + const char *ident, timestamp_t timestamp, int tz, > + const char *message, void *cb_data) > +{ > + int ret = 0; > + struct object_id *r_oid = cb_data; > + > + ret = oideq(n_oid, r_oid); > + if (!ret) { Rather than having the largest part of the actual code statements in this function indented, it would make more sense to write if (oideq(n_oid, r_oid)) return 1; if (!(local = lookup...) || !(remote = lookup...)) return 0; return in_merge_bases(remote, local); > + struct commit *loc = lookup_commit_reference(the_repository, > + n_oid); > + struct commit *rem = lookup_commit_reference(the_repository, > + r_oid); > + ret = (loc && rem) ? in_merge_bases(rem, loc) : 0; > + } This chooses the strategy of iterating over the reflog just once, and at every step first testing whether the respective reflog entry is identical to the remote-tracking branch tip. Only when they are different do we test whether the remote-tracking branch tip is at least reachable from the reflog entry. Let's assume that our local branch has 20 reflog entries, and the 4th one is identical to the current tip of the remote-tracking branch. Then we tested reachability 3 times. But that test is rather expensive. Therefore, I would have preferred to have a call to `for_each_reflog_ent_reverse()` with a callback function that only returns the `oideq()` result, and only if the return value of that call is 0, I would have wanted to see another call to `for_each_reflog_ent_reverse()` to go through, this time looking for reachability. > + > + return ret; > +} > + > +/* > + * Iterate through the reflog of a local branch and check > + * if the tip of the remote-tracking branch is reachable > + * from one of the entries. > + */ > +static int ref_reachable_from_reflog(const struct object_id *r_oid, > + const struct object_id *l_oid, > + const char *local_ref_name) > +{ > + int ret = 0; > + struct commit *r_commit, *l_commit; > + > + l_commit = lookup_commit_reference(the_repository, l_oid); > + r_commit = lookup_commit_reference(the_repository, r_oid); At this point, we already LOOked up `r_commit`. But we don't pass that to `ref_reachable()` at any point (instead passing only `r_oid`), so we have to perform the lookup again. That's wasteful. Shouldn't we pass `r_commit` directly? With the two-pass strategy I outlined above, the first pass would use `r_oid`, and only when the second pass is necessary would we resort to calling the expensive reachability check. > + > + /* > + * If the remote-tracking ref is an ancestor of the local > + * ref (a merge, for instance) there is no need to iterate > + * through the reflog entries to ensure reachability; it > + * can be skipped to return early instead. > + */ > + ret = (r_commit && l_commit) ? in_merge_bases(r_commit, l_commit) : 0; Correct me if I am wrong, but isn't the first reflog entry (`@{0}`) identical to `r_commit`? In that case, the first iteration of the second pass over the reflog would trivially perform this check, and we do not need to duplicate the logic here. > + if (!ret) > + ret = for_each_reflog_ent_reverse(local_ref_name, ref_reachable, > + (struct object_id *)r_oid); > + > + return ret; > +} > + > +/* > + * Check for reachability of a remote-tracking > + * ref in the reflog entries of its local ref. > + */ > +void check_reflog_for_ref(struct ref *r_ref) > +{ > + struct object_id r_oid; > + struct ref *l_ref = get_local_ref(r_ref->name); > + > + if (r_ref->if_includes && l_ref && !read_ref(l_ref->name, &r_oid)) If `r_ref->if_includes` is 0, we do not even have to get the local ref, correct? It would make `check_reflog_for_ref()` much easier to read for me if it was only called when that flag was already verified to be 1, and then followed this structure: if (!l_ref) return; if (read_ref(...)) warning(_("ignoring stale remote branch information ...")); else r_ref->unreachable = ... Also, it might make a lot more sense to rename `check_reflog_for_ref()` to `check_if_includes_upstream()`, and to rename `r_ref` to `local` and `l_ref` to `remote_tracking` or something like that: nothing is inherently "left" or "right" about those refs. > + r_ref->unreachable = !ref_reachable_from_reflog(&r_ref->old_oid, > + &r_oid, > + l_ref->name); > +} > + > static void apply_cas(struct push_cas_option *cas, > struct remote *remote, > struct ref *ref) > { > - int i; > + int i, is_tracking = 0; > > /* Find an explicit --