From: Srinidhi Kaushik Date: Sat, 19 Sep 2020 17:01:14 GMT Subject: Re: [PATCH v3 1/7] remote: add reflog check for "force-if-includes" Message-ID: <20200919170114.GA81372@mail.clickyotomy.dev> In-Reply-To: Hi Johannes, On 09/16/2020 14:35, Johannes Schindelin wrote: > > [...] > > + 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. OK, you're right about this. Will update check to use the two-step approach. > > [...] > > + 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? Hmm, I suppose we can. Not sure why I did that in the first place. > 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. That's a correct assumption; the second pass will check for this. > > + 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. Now that I think about it, we don't need te call to "read_ref()" at all because the reflog will have the OID we need for checking. As to "{l,r}_ref", they were meant to be [l]ocal and [r]emote. :) > > + 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 --