Re: [PATCH v4 20/25] sequencer: refactor write_message()

[Johannes Schindelin]

Hi Junio,

On Fri, 21 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> Ah, make that four steps.  The final one is:
> >> 
> >> - add append_eol parameter that nobody uses at this step in the
> >>   series.
> >>
> >> This is a new feature to the helper.  While it is OK to have it as a
> >> preparatory step in this series, it is easier to understand if it
> >> kept as a separate step.  It is even more preferrable if it is made
> >> as a preparatory step in a series that adds a caller that passes
> >> true to append_eol to this helper...
> >
> > Done,
> > Dscho
> 
> Hmm, what has been done exactly?  I still see append_eol in v5 where
> nobody uses it yet.  Confused...

Your bullet point suggests that this change should be a separate patch. I
did that.

And since this patch series' purpose is to prepare the sequencer for the
upcoming series that teaches it to understand git-rebase-todo scripts,
this patch falls fair and square into the preparatory phase.

Ciao,
Dscho

Re: [PATCH v4 20/25] sequencer: refactor write_message()

[Junio C Hamano]

Johannes Schindelin  writes:

>> Ah, make that four steps.  The final one is:
>> 
>> - add append_eol parameter that nobody uses at this step in the
>>   series.
>>
>> This is a new feature to the helper.  While it is OK to have it as a
>> preparatory step in this series, it is easier to understand if it
>> kept as a separate step.  It is even more preferrable if it is made
>> as a preparatory step in a series that adds a caller that passes
>> true to append_eol to this helper...
>
> Done,
> Dscho

Hmm, what has been done exactly?  I still see append_eol in v5 where
nobody uses it yet.  Confused...

Re: [PATCH v4 20/25] sequencer: refactor write_message()

[Johannes Schindelin]

Hi Junio,

On Thu, 20 Oct 2016, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > If I were doing this, I would make this into three separate steps:
> >
> > - move the strbuf_release(msgbuf) to the caller in
> >   do_pick_commit();
> >
> > - add the missing rollback_lock_file(), which is a bugfix; and
> >   then finally
> >
> > - allow the helper to take not a strbuf but  pair as
> >   parameters.
> >
> > The end result of this patch achieves two thirds of the above, but
> > especially given that write_message() only has two call sites in a
> > single function, I think it is OK and preferrable even to do all
> > three.
> 
> Ah, make that four steps.  The final one is:
> 
> - add append_eol parameter that nobody uses at this step in the
>   series.

Done,
Dscho

Re: [PATCH v4 20/25] sequencer: refactor write_message()

[Junio C Hamano]

Junio C Hamano  writes:

> If I were doing this, I would make this into three separate steps:
>
> - move the strbuf_release(msgbuf) to the caller in
>   do_pick_commit();
>
> - add the missing rollback_lock_file(), which is a bugfix; and
>   then finally
>
> - allow the helper to take not a strbuf but  pair as
>   parameters.
>
> The end result of this patch achieves two thirds of the above, but
> especially given that write_message() only has two call sites in a
> single function, I think it is OK and preferrable even to do all
> three.

Ah, make that four steps.  The final one is:

- add append_eol parameter that nobody uses at this step in the
  series.

This is a new feature to the helper.  While it is OK to have it as a
preparatory step in this series, it is easier to understand if it
kept as a separate step.  It is even more preferrable if it is made
as a preparatory step in a series that adds a caller that passes
true to append_eol to this helper, or if that real change is small
enough, part of that patch that adds such a caller, not as a
separate step.

Re: [PATCH v4 20/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 will make it easy to create a new convenience function
> write_file_gently() that does not die(); as some of the upcoming callers
> of this new function will want to append a newline character, we already
> add that flag as parameter to write_with_lock_file().
>
> While at it, roll back the locked files in case of failure, as pointed
> out by Hannes Sixt.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 25 -
>  1 file changed, 20 insertions(+), 5 deletions(-)

Once a helper function starts taking  pair, not a strbuf,
it becomes obvious that it does not make much sense to calling
strbuf_release() from the helper.  It is caller's job to do the
memory management of the strbuf it uses to pass information to the
helper, and the current api into write_message() is klunky.

If I were doing this, I would make this into three separate steps:

- move the strbuf_release(msgbuf) to the caller in
  do_pick_commit();

- add the missing rollback_lock_file(), which is a bugfix; and
  then finally

- allow the helper to take not a strbuf but  pair as
  parameters.

The end result of this patch achieves two thirds of the above, but
especially given that write_message() only has two call sites in a
single function, I think it is OK and preferrable even to do all
three.

[PATCH v4 20/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 will make it easy to create a new convenience function
write_file_gently() that does not die(); as some of the upcoming callers
of this new function will want to append a newline character, we already
add that flag as parameter to write_with_lock_file().

While at it, roll back the locked files in case of failure, as pointed
out by Hannes Sixt.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 914a576..baccee9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,22 +234,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 (commit_lock_file(_file) < 0)
+   if (write_in_full(msg_fd, buf, len) < 0) {
+   rollback_lock_file(_file);
+   return error_errno(_("Could not write to '%s'"), filename);
+   }
+   if (append_eol && write(msg_fd, "\n", 1) < 0) {
+   rollback_lock_file(_file);
+   return error_errno(_("Could not write eol to '%s"), filename);
+   }
+   if (commit_lock_file(_file) < 0) {
+   rollback_lock_file(_file);
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;
+}
+
 /*
  * 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.1.513.g00ef6dd