Subject: Re: [PATCH v2 21/25] sequencer: refactor write_message()

From: Junio C Hamano <[email-masked]>
Date: Sun, 11 Sep 2016 16:38:09 -0700

Johannes Schindelin <[email-masked]> writes:

> The write_message() function safely writes an strbuf to a file.
> Sometimes it is inconvenient to require an strbuf, though: the text to
> be written may not be stored in a strbuf, or the strbuf should not be
> released after writing.
>
> Let's refactor "safely writing string to a file" into
> write_with_lock_file(), and make write_message() use it. The new
> function makes it easy to create new convenience function
> write_file_gently(); as some of the upcoming callers of this new
> function would want to append a newline character, add a flag for it in
> write_file_gently(), and thus in write_with_lock_file().

Makes sense.  As an abstraction, "give me strbuf for my sole use,
and I'll trash its contents when I am done" feels like a horrible
helper interface; perhaps that was overly aggressively refactored by
noticing that then-current callers all released the strbuf, but
still feels wrong.

And this makes the underlying helper a lot more useful.

>
> Signed-off-by: Johannes Schindelin <[email-masked]>
> ---
>  sequencer.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5e5d113..aa949d4 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -242,22 +242,37 @@ static void print_advice(int show_hint, struct replay_opts *opts)
>  	}
>  }
>  
> -static int write_message(struct strbuf *msgbuf, const char *filename)
> +static int write_with_lock_file(const char *filename,
> +				const void *buf, size_t len, int append_eol)
>  {
>  	static struct lock_file msg_file;
>  
>  	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)
> -		return error_errno(_("Could not write to %s"), filename);
> -	strbuf_release(msgbuf);
> +	if (write_in_full(msg_fd, buf, len) < 0)
> +		return error_errno(_("Could not write to '%s'"), filename);
> +	if (append_eol && write(msg_fd, "\n", 1) < 0)
> +		return error_errno(_("Could not write eol to '%s"), filename);
>  	if (commit_lock_file(&msg_file) < 0)
>  		return error(_("Error wrapping up %s."), filename);
>  
>  	return 0;
>  }
>  
> +static int write_message(struct strbuf *msgbuf, const char *filename)
> +{
> +	int res = write_with_lock_file(filename, msgbuf->buf, msgbuf->len, 0);
> +	strbuf_release(msgbuf);
> +	return res;
> +}
> +
> +static int write_file_gently(const char *filename,
> +			     const char *text, int append_eol)
> +{
> +	return write_with_lock_file(filename, text, strlen(text), append_eol);
> +}
> +
>  /*
>   * Reads a file that was presumably written by a shell script, i.e.
>   * with an end-of-line marker that needs to be stripped.