On Mon, Dec 9, 2019 at 1:51 PM Junio C Hamano <gitster@xxxxxxxxx> 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.
> -- >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
