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-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-11.5 required=3.0 tests=AWL,BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE, USER_IN_DEF_DKIM_WL shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 95D391F463 for ; Tue, 26 Nov 2019 18:27:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726005AbfKZS1i (ORCPT ); Tue, 26 Nov 2019 13:27:38 -0500 Received: from mail-pl1-f202.google.com ([209.85.214.202]:53456 "EHLO mail-pl1-f202.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725933AbfKZS1i (ORCPT ); Tue, 26 Nov 2019 13:27:38 -0500 Received: by mail-pl1-f202.google.com with SMTP id p2so8232170plr.20 for ; Tue, 26 Nov 2019 10:27:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc:content-transfer-encoding; bh=qCtA40wBMcLnkbLF4zpcH2iZJrSbyotzQSvR32DrRAQ=; b=S31J+YzjPgqZQYM6oftaTEXaX8Zm6UqyIZve8VUv/9N3HkEtrr3Qi6/P+hz5ZdK0CM q39DAN9U/rH5x9qXdY1kxVUkxkAfJWa/+DDt8Zb71Pyp+IhU5/QtWxSo0Ud+w+bHe6SW lxROAPD9Fe59YqOv8orf7YfYsdfaIfcEecRBtjAHtxZGUvUeJ3ZCRYSgp4+ieXmIvqRV edN3DKIlx5+RMsy/tZxdoj8G1vsEhQvNyhLWW/sjMVwu9sBDDFVT3qfDFBXzQKVkBa0x o4RSPrcaG1tUQAX2Iy2Ceyc6icbs1ePZqtfG0MZ8dzyc7mHqKwNUjj8ZiFsvaAik39y4 oijg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc:content-transfer-encoding; bh=qCtA40wBMcLnkbLF4zpcH2iZJrSbyotzQSvR32DrRAQ=; b=GIr/ERvwmXE08inQFb9pqhpZT9bmZydO5xvDZ5rtrZCLdMr0I8EYCQu88A+VIl6gUR OCCw2FNLRHQNDCEPbWU2qnYxLJzgRQgT6eabj6HE9pFAuAKf+UGLguSBuH1OQru839QH 32VmPTxGwppeRVTYIt4AQSLP+sjG/qsPN7VmM7hUMK02MiNnRM2we2B8pMGdLMK8mo0v uQYkgiW/g79OF291BZrtkM2eSxO0hkmUJ+QEKAdzuw21Low1FlqzroG8tlRLDDqTj2TU vjSAR9KhmsEAN4FPO34hQbOJLQ2JmNAJMWlVNy4S/DIRDLNJHad6L4U6UDo7zNP6Kdne zZ9g== X-Gm-Message-State: APjAAAU2Ax3Jiz3c5Am0b0aFQIyXMH2COxS6uUAHP5jRBcaW00eByeZ8 f+XTA20Rrmu/E+Mkt8pMbUhPGwz/BgYZGtU3DrKP X-Google-Smtp-Source: APXvYqweu7BCUl4pg7ljtWJuKORbSScI7JHHgmppl7h8In8225pxwCADEJtQXmFzV2oLiAij4r6TRSFe/y2Zq22RmP8V X-Received: by 2002:a65:62cc:: with SMTP id m12mr20815532pgv.397.1574792857352; Tue, 26 Nov 2019 10:27:37 -0800 (PST) Date: Tue, 26 Nov 2019 10:27:34 -0800 In-Reply-To: <20191124174332.30887-5-alban.gruin@gmail.com> Message-Id: <20191126182734.142503-1-jonathantanmy@google.com> Mime-Version: 1.0 References: <20191124174332.30887-5-alban.gruin@gmail.com> X-Mailer: git-send-email 2.24.0.432.g9d3f5f5b63-goog Subject: Re: [PATCH v4 4/5] rebase: fill `squash_onto' in get_replay_opts() From: Jonathan Tan To: alban.gruin@gmail.com Cc: git@vger.kernel.org, Johannes.Schindelin@gmx.de, phillip.wood@dunelm.org.uk, gitster@pobox.com, jonathantanmy@google.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Thanks for the better commit message! Just one note: > When sequencer_continue() is called by complete_action(), `opts' has > been filled by get_replay_opts(). I searched for "get_replay_opts" in complete_action() but couldn't find it, and had to do some searching to realize that what you mean is that whenever complete_action() is called by do_interactive_rebase() in builtin/rebase.c (its only caller), the "opts" argument was filled by get_replay_opts(). Maybe reword as: When do_interactive_rebase() in builtin/rebase.c calls complete_action(), it passes an "opts" argument generated by get_replay_opts(). complete_action() then passes it to sequencer_continue(). > Currently, it does not initialise the > `squash_onto' field (used by the `--root' mode), only > read_populate_opts() does. It=E2=80=99s not a problem yet since > sequencer_continue() calls it before pick_commits(), but it would lead > to incorrect results once complete_action() is modified to call > pick_commits() directly. >=20 > Let=E2=80=99s change that. The rest is clear; thanks. I would like to make it explicit that pick_commits() uses "squash_onto", and make the antecedents of all the "it"s clear, so I would write it like this: Currently, get_replay_opts() does not initialize the "squash_onto" field (used by the "--root" mode); only read_populate_opts() does. This field is used by pick_commits(), called by sequencer_continue(). It's not a problem yet since sequencer_continue() currently calls read_populate_opts() before pick_commits(), but it would lead to incorrect results once complete_action() is modified to call pick_commits() directly (bypassing sequencer_continue() and, hence, read_populate_opts()). Let's change that. Also, I think it's better to use the regular quote ' instead of the smart quote =E2=80=99.