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

Hi Kuba,

On Thu, 1 Sep 2016, Jakub Narębski wrote:

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

quoted

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.

Rephrased.

quoted

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?

I don't know, but I took it.

quoted

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

Done.

quoted

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

Done.

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

quoted

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

As I inherited this message, I'll keep it.

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?

Thank you for your review!
Dscho