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.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,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 3698C1F4B4 for ; Wed, 23 Sep 2020 16:24:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726599AbgIWQYw (ORCPT ); Wed, 23 Sep 2020 12:24:52 -0400 Received: from pb-smtp20.pobox.com ([173.228.157.52]:50298 "EHLO pb-smtp20.pobox.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726184AbgIWQYv (ORCPT ); Wed, 23 Sep 2020 12:24:51 -0400 Received: from pb-smtp20.pobox.com (unknown [127.0.0.1]) by pb-smtp20.pobox.com (Postfix) with ESMTP id 585611068A3; Wed, 23 Sep 2020 12:24:48 -0400 (EDT) (envelope-from junio@pobox.com) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=ipIvTz1xXl/eNgbYKUJ+o7cs8ho=; b=agk8bd cF/obupWCps3DRKp9Y/0tYoPwyCvEVWmbjNaOs+7x0tiWeKeo+C+eC+vHN/0uYyA R2m0yxS8RkXI4yKNq/c9nkol/CXnbP4jzRvHaDvBUPpP85Ixr5Muozy3wt8KcA9+ lnrbGSgmLJDIDJWcNSnVd6VLWIIs8mgtfZLmc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=DRc1Nnhvxzs8CxzCskZ3KhCpod6m00yh BGO245nakJ9YympBelTDMp7tvlWMMjttD2n4e1r+mUrq+EEFwPa1dtH93tAAlebh /cpmaG6dpEKOL4u/D3KhPhKfN14wiIdXqkmavkx4hBS5pxsOJtv4/EEUrB2PdAyk TtwiY+/ku7w= Received: from pb-smtp20.sea.icgroup.com (unknown [127.0.0.1]) by pb-smtp20.pobox.com (Postfix) with ESMTP id 4FD371068A2; Wed, 23 Sep 2020 12:24:48 -0400 (EDT) (envelope-from junio@pobox.com) Received: from pobox.com (unknown [34.74.119.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp20.pobox.com (Postfix) with ESMTPSA id 92DDF10689F; Wed, 23 Sep 2020 12:24:45 -0400 (EDT) (envelope-from junio@pobox.com) From: Junio C Hamano To: Phillip Wood Cc: Srinidhi Kaushik , git@vger.kernel.org Subject: Re: [PATCH v5 1/3] push: add reflog check for "--force-if-includes" References: <20200919170316.5310-1-shrinidhi.kaushik@gmail.com> <20200923073022.61293-1-shrinidhi.kaushik@gmail.com> <20200923073022.61293-2-shrinidhi.kaushik@gmail.com> <3c254220-de10-8e17-1175-f88e790c17a6@gmail.com> Date: Wed, 23 Sep 2020 09:24:43 -0700 In-Reply-To: <3c254220-de10-8e17-1175-f88e790c17a6@gmail.com> (Phillip Wood's message of "Wed, 23 Sep 2020 11:18:43 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 4AEE8B06-FDB9-11EA-A981-F0EA2EB3C613-77302942!pb-smtp20.pobox.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Phillip Wood 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.