Re: [PATCH v2 2/2] rebase: find --fork-point with full ref
From: Denton Liu <hidden>
Date: 2019-12-06 01:48:07
Hi Alex, On Thu, Dec 05, 2019 at 06:57:04PM -0500, Alex Torok wrote:
quoted hunk ↗ jump to hunk
rebase --fork-point needs to look up the full ref name before calling get_fork_point in the same manner that merge-base --fork-point does. Signed-off-by: Alex Torok <redacted> --- builtin/rebase.c | 4 +++- t/t3431-rebase-fork-point.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)diff --git a/builtin/rebase.c b/builtin/rebase.c index e755087b0f..821994f676 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c@@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) struct commit *head = lookup_commit_reference(the_repository, &options.orig_head); + char * full_name;
nit: * should be attached to the variable name.
+ dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
Also, thinking about this more, would it be possible to put the dwim_ref logic into get_fork_point() directly? There are currently only these two callers so I suspect it should be fine and it'll result in cleaner logic. We could also squash it down into one patch.
quoted hunk ↗ jump to hunk
options.restrict_revision = - get_fork_point(options.upstream_name, head); + get_fork_point(full_name, head); } if (repo_read_index(the_repository) < 0)diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh index 78851b9a2a..6ecdae918e 100755 --- a/t/t3431-rebase-fork-point.sh +++ b/t/t3431-rebase-fork-point.sh@@ -49,6 +49,7 @@ test_rebase 'G F C D B A' --no-fork-point --onto D test_rebase 'G F C B A' --no-fork-point --keep-base test_rebase 'G F E D B A' --fork-point refs/heads/master test_rebase 'G F D B A' --fork-point --onto D refs/heads/master +test_rebase 'G F D B A' --fork-point --onto D master
It's not obvious why this was failing in the first place. Perhaps we could document it better in the commit message? Maybe something like: We used to pass in the upstream_name directly into the get_fork_point() machinery. However, get_fork_point() was expecting a fully qualified ref name even though most users use the short name for branches. This resulted in `--fork-point` not working as expected since, without the full ref name, the reflog lookup would fail and it would behave as if we weren't passing in `--fork-point` at all. Also, I'm not why this test case in particular that was duplicated (and not the one above) given that the first three `--fork-point` test cases fail without the change to rebase. Perhaps we want to duplicate all "refs/heads/master" tests with a corresponding "master" test? Thanks, Denton
test_rebase 'G F B A' --fork-point --keep-base refs/heads/master test_rebase 'G F C E D B A' refs/heads/master test_rebase 'G F C D B A' --onto D refs/heads/master -- 2.17.1