Alex Henrie <alexhenrie24@xxxxxxxxx> writes:
> - /* Make sure the branch to rebase onto is valid. */
So, we used to ...
> - if (keep_base) {
> - strbuf_reset(&buf);
> - strbuf_addstr(&buf, options.upstream_name);
> - strbuf_addstr(&buf, "...");
> - options.onto_name = xstrdup(buf.buf);
... store "<upstream>..." in onto_name ...
> - } else if (!options.onto_name)
> - options.onto_name = options.upstream_name;
... or used <upstream> as onto_name, and then ...
> - if (strstr(options.onto_name, "...")) {
... immediately used the "..." as a cue to compute the merge base
and ...
> - if (get_oid_mb(options.onto_name, &merge_base) < 0) {
> - if (keep_base)
> - die(_("'%s': need exactly one merge base with branch"),
> - options.upstream_name);
> - else
> - die(_("'%s': need exactly one merge base"),
> - options.onto_name);
> - }
> - options.onto = lookup_commit_or_die(&merge_base,
> - options.onto_name);
... used the "<upstream>..." as a label, and the merge base as the
"onto" commit.
> - } else {
> - options.onto =
> - lookup_commit_reference_by_name(options.onto_name);
> - if (!options.onto)
> - die(_("Does not point to a valid commit '%s'"),
> - options.onto_name);
> - }
> -
But that was done before we even examined the optinal "this one, not
the current branch, is to be rebased" argument. So it all works by
assuming the current branch is what is being rebase, which clearly
is wrong.
It is surprising that a basic and trivial bug was with us forever.
I wonder if the addition of --keep-base was faulty and the feature
was broken from the beginning, or there was an unrelated change that
broke the interaction between --keep-base and branch-to-be-rebased?
... goes and looks ...
Between 640f9cd5 (Merge branch 'dl/rebase-i-keep-base',
2019-09-30) which was where "--keep-base" was introduced, and
today's codebase, there are many unrelated changes to
builtin/rebase.c but this part is essentially unchanged. Before
the option was introduced, it didn't matter if the computation
of "onto" came before or after the next part that is not shown
in this patch, because "..." could have come only from the
end-user input and there the end-user must have given the branch
to be rebased on the right hand side of "..." if that is what
they meant. So, the feature was broken from the moment the
"--keep-base" option was introduced.
Now we got rid of all the above, so we need to be careful that we do
not depend on options.onto_name and options.onto in the part that
you did not touch, from here to the precontext of the next hunk.
And I looked for onto and onto_name, these strings do not appear
from this point until the next hunk where we see an added line, so
we are not breaking that part of the code by removing this block of
code from here and moving it down.
> /*
> * If the branch to rebase is given, that is the branch we will rebase
> * branch_name -- branch/commit being rebased, or
> @@ -1659,6 +1632,34 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> } else
> BUG("unexpected number of arguments left to parse");
>
> + /* Make sure the branch to rebase onto is valid. */
And now we do what the removed block wanted to do here, but we do so
in a different context. We know know which branch gets rebased in
the branch_name variable, so we do not use "<upstream>..." notation
to imply "the current branch" on the RHS; we can be explicit.
> + if (keep_base) {
> + strbuf_reset(&buf);
> + strbuf_addstr(&buf, options.upstream_name);
> + strbuf_addstr(&buf, "...");
> + strbuf_addstr(&buf, branch_name);
... and that is what happens here, which is good.
The rest of this new block is a literal copy from the old code
removed above, which is as expected.
> + options.onto_name = xstrdup(buf.buf);
> + } else if (!options.onto_name)
> + options.onto_name = options.upstream_name;
> + if (strstr(options.onto_name, "...")) {
> + if (get_oid_mb(options.onto_name, &merge_base) < 0) {
> + if (keep_base)
> + die(_("'%s': need exactly one merge base with branch"),
> + options.upstream_name);
> + else
> + die(_("'%s': need exactly one merge base"),
> + options.onto_name);
> + }
> + options.onto = lookup_commit_or_die(&merge_base,
> + options.onto_name);
> + } else {
> + options.onto =
> + lookup_commit_reference_by_name(options.onto_name);
> + if (!options.onto)
> + die(_("Does not point to a valid commit '%s'"),
> + options.onto_name);
> + }
> +
> if (options.fork_point > 0) {
> struct commit *head =
> lookup_commit_reference(the_repository,
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index 3716a42e81..d1db528e25 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -129,6 +129,22 @@ test_expect_success 'rebase --keep-base main from topic' '
> test_cmp expect actual
> '
>
> +test_expect_success 'rebase --keep-base main topic from main' '
> + git reset --hard &&
Clearing whatever cruft the last test left is good, but ...
> + git checkout topic &&
> + git reset --hard G &&
> + git checkout main &&
it would be more efficient to just do
git checkout main &&
git branch -f topic G &&
no? Instead of rewriting the working tree three times, you only
need to do so once here.
