Re: [PATCH v5 22/27] sequencer: teach write_message() to append an optional LF

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  writes:
> 
> > This commit prepares for future callers that will have a pointer/length
> > to some text to be written that lacks an LF, yet an LF is desired.
> > Instead of requiring the caller to append an LF to the buffer (and
> > potentially allocate memory to do so), the write_message() function
> > learns to append an LF at the end of the file.
> 
> As no existing callers need this, it probably is better left out and
> added to the series that actually needs the new feature as a
> preparatory step.

Apart from this patch series being semantically the right place
("prepare-sequencer"), there is also the following consideration:
The next patch series is already quite long. Taking this current patch
series into account, which started out as a 22-patch series and needed to
bloat by 25% through four subsequent iterations, it is probably not a wise
idea to move this patch to a patch series that already weighs 34 patches.

So I respectfully, and forcefully, disagree,
Dscho

Re: [PATCH v5 22/27] sequencer: teach write_message() to append an optional LF

[Junio C Hamano]

Johannes Schindelin  writes:

> This commit prepares for future callers that will have a pointer/length
> to some text to be written that lacks an LF, yet an LF is desired.
> Instead of requiring the caller to append an LF to the buffer (and
> potentially allocate memory to do so), the write_message() function
> learns to append an LF at the end of the file.

As no existing callers need this, it probably is better left out and
added to the series that actually needs the new feature as a
preparatory step.

[PATCH v5 22/27] sequencer: teach write_message() to append an optional LF

[Johannes Schindelin]

This commit prepares for future callers that will have a pointer/length
to some text to be written that lacks an LF, yet an LF is desired.
Instead of requiring the caller to append an LF to the buffer (and
potentially allocate memory to do so), the write_message() function
learns to append an LF at the end of the file.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 300952f..000ce3e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,7 +234,8 @@ static void print_advice(int show_hint, struct replay_opts 
*opts)
}
 }
 
-static int write_message(const void *buf, size_t len, const char *filename)
+static int write_message(const void *buf, size_t len, const char *filename,
+int append_eol)
 {
static struct lock_file msg_file;
 
@@ -245,6 +246,10 @@ static int write_message(const void *buf, size_t len, 
const char *filename)
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);
@@ -748,13 +753,13 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
if (res < 0)
return res;
res |= write_message(msgbuf.buf, msgbuf.len,
-git_path_merge_msg());
+git_path_merge_msg(), 0);
} else {
struct commit_list *common = NULL;
struct commit_list *remotes = NULL;
 
res = write_message(msgbuf.buf, msgbuf.len,
-   git_path_merge_msg());
+   git_path_merge_msg(), 0);
 
commit_list_insert(base, );
commit_list_insert(next, );
-- 
2.10.1.583.g721a9e0