From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 24 Aug 2016 07:09:17 GMT
Subject: Re: [PATCH 01/15] sequencer: lib'ify write_message()
Message-ID: <CAPig+cSX_F-vQ_zSrTU6BQSfMeU=5V3krE7xgmBL=g=Jr8i2MA@mail.gmail.com>
In-Reply-To: <6dc5e927cbdd1847db5b6cd75041609dd1e1f76d.1471968378.git.johannes.schindelin@gmx.de>

On Tue, Aug 23, 2016 at 12:06 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct replay_opts *opts)
> -static void write_message(struct strbuf *msgbuf, const char *filename)
> +static int write_message(struct strbuf *msgbuf, const char *filename)
>  {
>         static struct lock_file msg_file;
>
> -       int msg_fd = hold_lock_file_for_update(&msg_file, filename,
> -                                              LOCK_DIE_ON_ERROR);
> +       int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
> +       if (msg_fd < 0)
> +               return error_errno(_("Could not lock '%s'"), filename);
>         if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> -               die_errno(_("Could not write to %s"), filename);
> +               return error_errno(_("Could not write to %s"), filename);

Does this need to rollback the lockfile before returning[*]?

[*] I'm not terribly familiar with the lockfile mechanism and I don't
have a lot of time to study it presently, so ignore me if this is a
stupid question.

>         strbuf_release(msgbuf);
>         if (commit_lock_file(&msg_file) < 0)
> -               die(_("Error wrapping up %s."), filename);
> +               return error(_("Error wrapping up %s."), filename);
> +
> +       return 0;
>  }