Re: [PATCH] sequencer: reschedule pick if index can't be locked
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
>
