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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2016, 20:26 UTC
Message-ID: <xmqqd1iug42f.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqh986g49i.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 15 quoted lines

> 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.

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.