Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname
From: Alex Torok <hidden>
Date: 2019-12-11 01:21:22
Possibly related (same subject, not in this thread)
- 2019-12-09 · [PATCH v3 1/1] rebase: fix --fork-point with short refname · Alex Torok <hidden>
On Mon, Dec 9, 2019 at 1:51 PM Junio C Hamano [off-list ref] wrote:
and there are other calls to die(_("...")) in the caller everywhere,
so I'd say you are over-engineering a simple bugfix.
Wouldn't it be sufficient for this fix to be more like this?This is essentially what I had done in the v2 version of my patch with the only "big" difference being pulling out the dwim_ref() switching and dying logic into a dwim_ref_or_die() function. Let me know if you want me to adjust my patch at all (my v2 patch is missing a call to free()...). If the patch that you replied here with is sufficient for how you would fix it, I'm fine with you just using that.
quoted hunk ↗ jump to hunk
-- >8 -- Subject: rebase: --fork-point regression fix "git rebase --fork-point master" used to work OK, as it internally called "git merge-base --fork-point" that knew how to handle short refname and dwim it to the full refname before calling the underlying get_fork_point() function. This is no longer true after the command was rewritten in C, as its internall call made directly to get_fork_point() does not dwim a short ref. Move the "dwim the refname argument to the full refname" logic that is used in "git merge-base" to the underlying get_fork_point() function, so that the other caller of the function in the implementation of "git rebase" behaves the same way to fix this regression. --- builtin/merge-base.c | 12 +----------- commit.c | 15 +++++++++++++-- t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 13 deletions(-)diff --git a/builtin/merge-base.c b/builtin/merge-base.c index e3f8da13b6..6719ac198d 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c@@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv) static int handle_fork_point(int argc, const char **argv) { struct object_id oid; - char *refname; struct commit *derived, *fork_point; const char *commitname; - switch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) { - case 0: - die("No such ref: '%s'", argv[0]); - case 1: - break; /* good */ - default: - die("Ambiguous refname: '%s'", argv[0]); - } - commitname = (argc == 2) ? argv[1] : "HEAD"; if (get_oid(commitname, &oid)) die("Not a valid object name: '%s'", commitname); derived = lookup_commit_reference(the_repository, &oid); - fork_point = get_fork_point(refname, derived); + fork_point = get_fork_point(argv[0], derived); if (!fork_point) return 1;diff --git a/commit.c b/commit.c index 40890ae7ce..016f14fe95 100644 --- a/commit.c +++ b/commit.c@@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) struct commit_list *bases; int i; struct commit *ret = NULL; + char *full_refname; + + switch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) { + case 0: + die("No such ref: '%s'", refname); + case 1: + break; /* good */ + default: + die("Ambiguous refname: '%s'", refname); + } memset(&revs, 0, sizeof(revs)); revs.initial = 1; - for_each_reflog_ent(refname, collect_one_reflog_ent, &revs); + for_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs); - if (!revs.nr && !get_oid(refname, &oid)) + if (!revs.nr) add_one_commit(&oid, &revs); for (i = 0; i < revs.nr; i++)@@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) cleanup_return: free_commit_list(bases); + free(full_refname); return ret; }diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh index 78851b9a2a..5b09aecd13 100755 --- a/t/t3431-rebase-fork-point.sh +++ b/t/t3431-rebase-fork-point.sh@@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base test_rebase 'G F C E D B A' --no-fork-point 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 E D B A' --fork-point 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 + test_rebase 'G F B A' --fork-point --keep-base refs/heads/master +test_rebase 'G F B A' --fork-point --keep-base master + test_rebase 'G F C E D B A' refs/heads/master +test_rebase 'G F C E D B A' master + test_rebase 'G F C D B A' --onto D refs/heads/master +test_rebase 'G F C D B A' --onto D master + test_rebase 'G F C B A' --keep-base refs/heads/master +test_rebase 'G F C B A' --keep-base master + +test_expect_success "git rebase --fork-point with ambigous refname" " + git checkout master && + git checkout -b one && + git checkout side && + git tag one && + test_must_fail git rebase --fork-point --onto D one +" test_done