Re: [PATCH 22/22] sequencer: refactor write_message()

From: Jakub Narębski <jnareb@gmail.com>
Date: Sep 1, 2016, 11:10 UTC
Message-ID: <4c0ee8f6-1302-1dcc-602d-7a84150ec39d@gmail.com>
In-Reply-To: <cb4253698ae3eca066c031e0aec4e83ede1fa3e5.1472457609.git.johannes.schindelin@gmx.de>

Hello Johannes,

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> The write_message() function safely writes an strbuf to a file.
> Sometimes this is inconvenient, though: the text to be written may not
> be stored in a strbuf, or the strbuf should not be released after
> writing.

By "this" you mean "using strbuf", isn't it? It is not very obvious, and I think it would be better to say it explicitly.

Show 5 quoted lines

>
> Let's allow for such use cases by refactoring write_message() to allow
> for a convenience function write_file_gently(). As some of the upcoming
> callers of that new function will want to append a newline character,
> let's just add a flag for that, too.

This paragraph feels a bit convoluted.

As I understand it, you 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().

Isn't it better / easier to understand?

Show 26 quoted lines

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 5efed2e..f5b5e5e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -239,22 +239,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)
> +	if (write_in_full(msg_fd, buf, len) < 0)
>  		return error_errno(_("Could not write to %s"), filename);

You could have, for consistency, add quotes around filename (see previous error_errno callsite), *while at it*:

  		return error_errno(_("Could not write to '%s'"), filename);

> -	strbuf_release(msgbuf);
> +	if (append_eol && write(msg_fd, "\n", 1) < 0)
> +		return error_errno(_("Could not write eol to %s"), filename);

Same here, and it wouldn't even be 'while at it'

  +		return error_errno(_("Could not write eol to '%s'"), filename);

>  	if (commit_lock_file(&msg_file) < 0)
>  		return error(_("Error wrapping up %s."), filename);

Another "while at it"... though the one that can be safely postponed (well, the make message easier to understand part, not the quote filename part):

  		return error(_("Error wrapping up writing to '%s'."), filename);

Show 10 quoted lines

>  
>  	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;
> +}

Nice.

Show 6 quoted lines

> +
> +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);
> +}

Nice. And it is static function, so we don't need to come up with a better function name (to describe its function better).

Show 5 quoted lines

> +
>  /*
>   * Reads a file that was presumably written by a shell script, i.e.
>   * with an end-of-line marker that needs to be stripped.
>

And thus we got to the last patch in this series. I have skipped patches that already got reviewed; are there some that you would like to have second review of? Is there patch series that needs to be applied earlier that needs a review?

P.S. I'll try to respond to your comments later today.

Regards,

-- 
Jakub Narębski