From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id F03AE1F4B4 for ; Sat, 19 Sep 2020 17:01:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726518AbgISRBX (ORCPT ); Sat, 19 Sep 2020 13:01:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48026 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726434AbgISRBX (ORCPT ); Sat, 19 Sep 2020 13:01:23 -0400 Received: from mail-pl1-x644.google.com (mail-pl1-x644.google.com [IPv6:2607:f8b0:4864:20::644]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5910AC0613CE for ; Sat, 19 Sep 2020 10:01:23 -0700 (PDT) Received: by mail-pl1-x644.google.com with SMTP id u9so4653006plk.4 for ; Sat, 19 Sep 2020 10:01:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=yhQ/11j5tSfboTkEHEwmOzQGadJWpLCGHcWMexYDYeY=; b=AAnZAn3J6ul34DBmxN36slD+jqthgI8BR1lautZjXMMdzNghhCvo0FToQ+VjF7s2Lr wZKl2S6tD1j64ir9SYx0AVoif7fGL7zlgQB2QUzvH52dWDVx9uUlR8JpLDjiTvVXv7Is /LWMPEg9pmxH5Ie0/ZdgVUORatvyAfkkSYiKh6wQFMlDnqVmRVlrHju2UAX476Vkdwrv PobHANbGbEB88a/zboLrSGDDfRYtiIqi0NZ+25qx4eb+D3xu944g0T1pdA7W3ZSZT35C dYdsFNpP32T+Ngm2KqRThSmnT/RKbuxhB7PZWhsVoAVJqk9SFBTONirSWuE4VNjLjtpD tSpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=yhQ/11j5tSfboTkEHEwmOzQGadJWpLCGHcWMexYDYeY=; b=p+W8rKA9UtPoW7H/L6JJNRr6lDU/Vg79BoJDrAAQWF/mbJai5DVWqyUR90fgOWEj5C c3ZACv/aMAeyYBgVU3+aVha8HEq7fP11QmwPONx8u22D7hE0HTIrDIwdY1SLpESk9qcZ 8+lfYA3KrsvvKuE9HNzqe4XSGv9p2bFf2RdIb3bSDB81MFjmoF9IzyTQPNQquqgwX8CV lfsCI5xutQnu5soNvnCPkjT6Dd3DvxyuCGMqgt+UR+Jz9zkHXVuJsFbToiUsg0NIZ+V5 ejTWlIr4UXvhRnCxWBa0dJ1vt+gcmsLF+ySh91+l/6/eLeHwHAiKrvwMqBHEnMGsUI3E VNdA== X-Gm-Message-State: AOAM532XEDM39aaUg6d9NHRI79qONyZqvv4jrlrl/SaR6ErlzZvnjk1Z 6mIH0JbsA27YtL+tMXQYGn8= X-Google-Smtp-Source: ABdhPJxUXo/MGPHISLNMUjMzJCyHMjUhfq38tI1vJuU/srG/HAh5x0oQrY6xpMby0bm8fD3D+CYo7g== X-Received: by 2002:a17:90a:760f:: with SMTP id s15mr17897166pjk.214.1600534882527; Sat, 19 Sep 2020 10:01:22 -0700 (PDT) Received: from mail.clickyotomy.dev ([104.200.132.172]) by smtp.gmail.com with ESMTPSA id u71sm7508524pfc.43.2020.09.19.10.01.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 19 Sep 2020 10:01:21 -0700 (PDT) Date: Sat, 19 Sep 2020 22:31:14 +0530 From: Srinidhi Kaushik To: Johannes Schindelin Cc: git@vger.kernel.org Subject: Re: [PATCH v3 1/7] remote: add reflog check for "force-if-includes" Message-ID: <20200919170114.GA81372@mail.clickyotomy.dev> References: <20200912150459.8282-1-shrinidhi.kaushik@gmail.com> <20200913145413.18351-1-shrinidhi.kaushik@gmail.com> <20200913145413.18351-2-shrinidhi.kaushik@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --