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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Denton Liu <liu.denton@gmail.com>, Alex Torok <alext9@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 2/2] rebase: find --fork-point with full ref
Date: Fri, 6 Dec 2019 10:52:31 +0000	[thread overview]
Message-ID: <e898ad43-5649-8f4c-7c93-fab09197fc92@gmail.com> (raw)
In-Reply-To: <20191206014828.GB16183@generichostname>

Hi Alex

Thanks for working on this

On 06/12/2019 01:48, Denton Liu wrote:
> 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.

I think you also need to free it once you've called get_fork_point() as 
well.

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

If you do that then it would be better to use error() rather than die() 
in get_fork_point() and return an error to the caller as we try to avoid 
adding code to libgit that dies. This lets the caller handle any cleanup 
that they need to before exiting.

Best Wishes

Phillip

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