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

[Johannes Schindelin]

Hi Junio & Hannes,

On Thu, 15 Sep 2016, Junio C Hamano wrote:

> Johannes Sixt  writes:
> 
> > Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:
> >> -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(_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(_file) < 0)
> >>return error(_("Error wrapping up %s."), filename);
> >>
> >>return 0;
> >>  }
> >
> > The two error paths in the added lines should both
> >
> > rollback_lock_file(_file);
> >
> > , I think. But I do notice that this is not exactly new, so...
> 
> It may not be new for this step, but overall the series is aiming to
> libify the stuff, so we should fix fd and lockfile leaks like this
> as we notice them.

Makes sense, even for the final commit_lock_file().

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Sixt  writes:

> Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:
>> -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(_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(_file) < 0)
>>  return error(_("Error wrapping up %s."), filename);
>>
>>  return 0;
>>  }
>
> The two error paths in the added lines should both
>
>   rollback_lock_file(_file);
>
> , I think. But I do notice that this is not exactly new, so...

It may not be new for this step, but overall the series is aiming to
libify the stuff, so we should fix fd and lockfile leaks like this
as we notice them.

Thanks.

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

[Johannes Sixt]

Am 11.09.2016 um 12:55 schrieb Johannes Schindelin:

-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(_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(_file) < 0)
return error(_("Error wrapping up %s."), filename);

return 0;
 }


The two error paths in the added lines should both

rollback_lock_file(_file);

, I think. But I do notice that this is not exactly new, so...

-- Hannes

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

[Junio C Hamano]

Johannes Schindelin  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 
> ---
>  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(_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(_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.

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

[Johannes Schindelin]

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

Signed-off-by: Johannes Schindelin 
---
 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(_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(_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.
-- 
2.10.0.windows.1.10.g803177d