Re: [PATCH v4 20/25] sequencer: refactor write_message()
Hi Junio, On Fri, 21 Oct 2016, Junio C Hamano wrote: > Johannes Schindelinwrites: > > >> 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 Schindelinwrites: >> 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 Hamanowrites: > > > 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 Hamanowrites: > 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 Schindelinwrites: > 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.