From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 06 Dec 2019 19:35:32 GMT
Subject: Re: [PATCH v2 2/2] rebase: find --fork-point with full refgg
Message-ID: <117ce276-f37b-057c-849f-909852a6dd2c@gmail.com>
In-Reply-To: <20191206191147.GA16379@generichostname>

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