From: Phillip Wood Date: Thu, 16 Nov 2017 10:43:48 GMT Subject: Re: [PATCH] sequencer: reschedule pick if index can't be locked Message-ID: <804e30dd-3c06-02c1-e5c0-f07265635943@talktalk.net> In-Reply-To: On 15/11/17 18:44, Martin Ågren wrote: > On 15 November 2017 at 11:41, Phillip Wood wrote: >> From: Phillip Wood >> >> 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 >> --- >> 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 >