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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2016, 20:22 UTC
Message-ID: <xmqqh986g49i.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <9a66d53ebd86d35136597e534be03605bb1346bc.1476450940.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 20 quoted lines

> 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 <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)

Once a helper function starts taking <buf, len> 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 <buf, len> 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.