Re: [PATCH v2 2/2] rebase: find --fork-point with full ref

From: Denton Liu <liu.denton@gmail.com>
To: Alex Torok <alext9@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 2/2] rebase: find --fork-point with full ref
Date: Thu, 5 Dec 2019 17:48:28 -0800	[thread overview]
Message-ID: <20191206014828.GB16183@generichostname> (raw)
In-Reply-To: <20191205235704.31385-3-alext9@gmail.com>

Hi Alex,

On Thu, Dec 05, 2019 at 06:57:04PM -0500, Alex Torok wrote:
> 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 <alext9@gmail.com>
> ---
>  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.

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