Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname

[Denton Liu <liu.denton@gmail.com>]

Hi Alex,

There are a couple of small issues with the patch below. If you iterate
on it, here are some small suggestions:

On Mon, Dec 09, 2019 at 10:51:47AM -0800, Junio C Hamano wrote:
> 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]);
> -	}
> -

In the unabridged v3, you cleaned this up by marking it for translation
and also lowercasing the first letter of the sentence. That was good.

If you iterate on this, could you create a preparatory patch before this
one that gives the same treatment to the other strings in merge-base? I
count 8 instances of die() being called in merge-base so I think that
the cleanup shouldn't be too arduous.

If we have a preparatory patch before this one, this patch will mostly
be pure code movement which would be nice.

>  	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);
> +	}

Yeah, we should fix these strings in the next iteration.

>  
>  	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
> +"

nit: use double-quotes instead of single-quotes to surround both the
test case name and the actual code itself.

Thanks,

Denton

>  
>  test_done