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

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

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

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