Re: [PATCH] sequencer: reschedule pick if index can't be locked

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Nov 16, 2017, 10:43 UTC
Message-ID: <804e30dd-3c06-02c1-e5c0-f07265635943@talktalk.net>
In-Reply-To: <CAN0heSrbszhhFauYHNs70-WWk+bju0sSVzjavRcwg09CzCXSjA@mail.gmail.com>

On 15/11/17 18:44, Martin Ågren wrote:

Show 35 quoted lines

> On 15 November 2017 at 11:41, Phillip Wood <phillip.wood@talktalk.net> wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Return an error instead of dying if the index cannot be locked in
>> do_recursive_merge() as if the commit cannot be picked it needs to be
>> rescheduled when performing an interactive rebase. If the pick is not
>> rescheduled and the user runs 'git rebase --continue' rather than 'git
>> rebase --abort' then the commit gets silently dropped.
> 
> Makes sense. (Your analysis, not the current behavior. ;-) )
> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   sequencer.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>>          char **xopt;
>>          static struct lock_file index_lock;
>>
>> -       hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
>> +       if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR))
>> +               return -1;
>>
>>          read_cache();
> 
>  From the commit message, I would have expected the flags to be zero. This patch
> does not only turn off the die-ing, it also tells the lockfile-API to print an
> error message before returning. I don't have an opinion on whether that extra
> verboseness is good or bad, but if it's wanted, I think the commit message
> should mention this change.

Hi Martin, thanks for your comments. LOCK_DIE_ON_ERROR also prints the same warning so that behavior is unchanged by this patch, though mentioning it in the commit message would be no bad thing.

> 
> Also, don't you want to check "< 0" rather than "!= 0"? If all goes
> well, the return value will be a file descriptor. I think that it will
> always be non-zero, so I think you'll always return -1 here.

Yes you're right, thanks. I thought I had tested this but I now realise my so called test just fast-forwarded so didn't touch this code path

Best Wishes

Phillip

> Martin
>