[v2,2/2] rebase: find --fork-point with full ref
diff mbox series

Message ID 20191205235704.31385-3-alext9@gmail.com
State New
Headers show
Series
  • rebase: fix bug in --fork-point
Related show

Commit Message

Alex Torok Dec. 5, 2019, 11:57 p.m. UTC
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(-)

Comments

Denton Liu Dec. 6, 2019, 1:48 a.m. UTC | #1
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
>
Phillip Wood Dec. 6, 2019, 10:52 a.m. UTC | #2
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
>>
Alex Torok Dec. 6, 2019, 1:46 p.m. UTC | #3
Thank you for the feedback Denton & Phillip!

On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 06/12/2019 01:48, Denton Liu wrote:
> > 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.

Yup. Got it.

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

Would the signature of get_fork_point change to be something like:
int get_fork_point(const char *refname, struct commit *commit,
   struct commit **fork_point, struct strbuf *err)

If not, could you point me to an example of some existing code
that does what you're talking about?


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

Sounds good.

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

I only duplicated one so that there would only be one test case that
would fail if a regression around getting the fork point with a short
ref name was introduced.

I just happened to pick that one because it was closest to the rebase
command I was running when I found the bug :)

I can include some of the above reasoning in the commit message.
Alternatively:
* I could duplicate all of tests
* I could change all of the tests to use the short ref name

I'm leaning towards just leaving one test (maybe with a comment?)
for the short ref name --fork-point so that there is more resolution
around where a bug could be on test failure.

Let me know what you think,
Alex
Denton Liu Dec. 6, 2019, 7:11 p.m. UTC | #4
Hi Alex,

On Fri, Dec 06, 2019 at 08:46:29AM -0500, Alex Torok wrote:
> Thank you for the feedback Denton & Phillip!
> 
> On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > On 06/12/2019 01:48, Denton Liu wrote:
> > > 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.
> 
> Yup. Got it.
> 
> > On 06/12/2019 01:48, Denton Liu wrote:
> > >
> > >> +            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.
> 
> Would the signature of get_fork_point change to be something like:
> int get_fork_point(const char *refname, struct commit *commit,
>    struct commit **fork_point, struct strbuf *err)

I would drop the last parameter. If an error is detected, you could just
do

	return error(_("oh no, something bad happened"));

Even though we try and avoid dying in the middle of libgit, we print
errors out very often so it should be fine here.

> > > 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?
> 
> I only duplicated one so that there would only be one test case that
> would fail if a regression around getting the fork point with a short
> ref name was introduced.
> 
> I just happened to pick that one because it was closest to the rebase
> command I was running when I found the bug :)
> 
> I can include some of the above reasoning in the commit message.
> Alternatively:
> * I could duplicate all of tests
> * I could change all of the tests to use the short ref name
> 
> I'm leaning towards just leaving one test (maybe with a comment?)
> for the short ref name --fork-point so that there is more resolution
> around where a bug could be on test failure.

I would just duplicate all of the tests. When the tests are pretty cheap
to run (as they are in this case), I tend to err on the side of adding
more tests since they might catch more odd edge-cases but, in this case,
all of the fork point logic goes through one common block so the
duplicate logic doesn't really buy us anything.

I'm pretty impartial so I'll leave it up to you ;)

Thanks,

Denton

> 
> Let me know what you think,
> Alex
Phillip Wood Dec. 6, 2019, 7:35 p.m. UTC | #5
On 06/12/2019 19:11, Denton Liu wrote:
> Hi Alex,
> 
> On Fri, Dec 06, 2019 at 08:46:29AM -0500, Alex Torok wrote:
>> Thank you for the feedback Denton & Phillip!
>>
>> On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> On 06/12/2019 01:48, Denton Liu wrote:
>>>> 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.
>>
>> Yup. Got it.
>>
>>> On 06/12/2019 01:48, Denton Liu wrote:
>>>>
>>>>> +            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.
>>
>> Would the signature of get_fork_point change to be something like:
>> int get_fork_point(const char *refname, struct commit *commit,
>>     struct commit **fork_point, struct strbuf *err)
> 
> I would drop the last parameter. If an error is detected, you could just
> do
> 
> 	return error(_("oh no, something bad happened"));
> 
> Even though we try and avoid dying in the middle of libgit, we print
> errors out very often so it should be fine here.

Yes that was what I was thinking of

Best Wishes

Phillip


> 
>>>> 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?
>>
>> I only duplicated one so that there would only be one test case that
>> would fail if a regression around getting the fork point with a short
>> ref name was introduced.
>>
>> I just happened to pick that one because it was closest to the rebase
>> command I was running when I found the bug :)
>>
>> I can include some of the above reasoning in the commit message.
>> Alternatively:
>> * I could duplicate all of tests
>> * I could change all of the tests to use the short ref name
>>
>> I'm leaning towards just leaving one test (maybe with a comment?)
>> for the short ref name --fork-point so that there is more resolution
>> around where a bug could be on test failure.
> 
> I would just duplicate all of the tests. When the tests are pretty cheap
> to run (as they are in this case), I tend to err on the side of adding
> more tests since they might catch more odd edge-cases but, in this case,
> all of the fork point logic goes through one common block so the
> duplicate logic doesn't really buy us anything.
> 
> I'm pretty impartial so I'll leave it up to you ;)
> 
> Thanks,
> 
> Denton
> 
>>
>> Let me know what you think,
>> Alex

Patch
diff mbox series

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;
+		dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
 		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
 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