Re: [PATCH v4 20/25] sequencer: refactor write_message()
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()
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()
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 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()
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()
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(&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) - return error_errno(_("Could not write to %s"), filename); - strbuf_release(msgbuf); - if (commit_lock_file(&msg_file) < 0) + if (write_in_full(msg_fd, buf, len) < 0) { + rollback_lock_file(&msg_file); + return error_errno(_("Could not write to '%s'"), filename); + } + if (append_eol && write(msg_fd, "\n", 1) < 0) { + rollback_lock_file(&msg_file); + return error_errno(_("Could not write eol to '%s"), filename); + } + if (commit_lock_file(&msg_file) < 0) { + rollback_lock_file(&msg_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