Subject: Re: [GSoC][PATCH 1/3] sequencer: add advice for revert

From: Phillip Wood <[email-masked]>
Date: Mon, 10 Jun 2019 18:46:50 +0100

Hi Rohit

On 10/06/2019 14:25, Rohit Ashiwal wrote:
> Hi Phillip
> 
> On 2019-06-10 10:39 UTC Phillip Wood <[email-masked]> wrote:
>>
>> Hi Rohit
>>
>> On 10/06/2019 06:13, Rohit Ashiwal wrote:
>>>
>>> [...]
>>> Firstly, signature of `create_seq_dir` doesn't allow us to call
>>> `sequencer_get_last_command()`. Changing that for the sake of a
>>> better error message is too much task for this patch as it is a
>>> subject of discussion on its own.
>>
>> There is only one caller and it already has a struct repository pointer
>> so it is a two line change, one of which is the insertion of a single
>> character to change create_seq_dir() so it can call
>> sequencer_get_last_command(). It is normal to change function signatures
>> (especially for static functions like this) when making changes, it is
>> part of improving the code base. The quality of error messages is
>> important to the overall user experience. It's when things go wrong that
>> users need accurate advice about what to do.
> 
> Nice! I'll do this in next revision then.

I'm glad that helped

> 
>>> (Also changing signature only
>>> makes sense if this patch series gets merged). FWIW, I think we
>>> should left this to further discussions for now and decide what
>>> to do later on.
>>
>> It is only a small change so why not do it now rather than putting it
>> off for another series which will be more work in the long run.
> 
> I'm fine with changing that here.

That's great

Best Wishes

Phillip

> Thanks
> Rohit