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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Cc: git@vger.kernel.org, newren@gmail.com, t.gummerer@gmail.com
Subject: Re: [GSoC][PATCH 1/3] sequencer: add advice for revert
Date: Mon, 10 Jun 2019 11:39:58 +0100	[thread overview]
Message-ID: <d59c078c-61da-1c3a-d215-80c9e3d08896@gmail.com> (raw)
In-Reply-To: <20190610051313.16222-1-rohit.ashiwal265@gmail.com>

Hi Rohit

On 10/06/2019 06:13, Rohit Ashiwal wrote:
> Hi Philip
> 
> On 2019-06-09 17:52 UTC Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Rohit
>>
>> Congratulations on your first GSoC patch series!
> 
> Thank you very much :)
> 
>> On 08/06/2019 20:19, Rohit Ashiwal wrote:
>>> [...]
>>> @@ -2655,6 +2655,7 @@ static int create_seq_dir(void)
>>>  	if (file_exists(git_path_seq_dir())) {
>>>  		error(_("a cherry-pick or revert is already in progress"));
>>>  		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
>>> +		advise(_("or  \"git revert (--continue | --quit | --abort)\""));
>>
>> I agree that it's a good idea to add advice for revert as well, but it
>> would be better to call sequencer_get_last_command() to find out if
>> we're running cherry-pick or revert and tailor the advice appropriately.
> 
> 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.

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

Best Wishes

Phillip

> 
>> Best Wishes
>>
>> Phillip
> 
> Thanks
> Rohit
> 