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

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 15, 2017, 18:44 UTC
Message-ID: <CAN0heSrbszhhFauYHNs70-WWk+bju0sSVzjavRcwg09CzCXSjA@mail.gmail.com>
In-Reply-To: <20171115104125.1686-1-phillip.wood@talktalk.net>

On 15 November 2017 at 11:41, Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 7 quoted lines

> 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. ;-) )

Show 18 quoted lines

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

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.

Martin