From: Phillip Wood Date: Tue, 30 Aug 2022 15:03:11 GMT Subject: Re: [PATCH 3/5] rebase: factor out merge_base calculation Message-ID: In-Reply-To: <20220824220251.1682480-1-jonathantanmy@google.com> Hi Jonathan Thanks for taking a look at this series On 24/08/2022 23:02, Jonathan Tan wrote: > "Phillip Wood via GitGitGadget" writes: >> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> die(_("Does not point to a valid commit '%s'"), >> options.onto_name); >> } >> - >> + if (keep_base) { >> + oidcpy(&merge_base, &options.onto->object.oid); >> + } else { >> + fill_merge_base(&options, &merge_base); >> + } >> if (options.fork_point > 0) >> options.restrict_revision = >> get_fork_point(options.upstream_name, options.orig_head); > > This patch makes sense, except for this part: why is fill_merge_base() > only called for non-keep_base, but it seemed to be unconditionally > called before? For what it's worth, all tests pass even with this diff: It's an optimization, we have already calculated the merge base above in the "onto" calculation when --keep-base is given. This is what I meant by "avoid calculating the merge-base twice when --keep-base is given" in the commit message, maybe I should try and come up with some clearer wording. Best Wishes Phillip > - if (keep_base) { > - oidcpy(&merge_base, &options.onto->object.oid); > - } else { > - fill_merge_base(&options, &merge_base); > - } > + fill_merge_base(&options, &merge_base);