Re: [PATCH v2 2/6] builtin/am: make sure state files are text

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2015, 16:19 UTC
Message-ID: <xmqqtwrn1gu6.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150824235547.GB13261@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

> On Mon, Aug 24, 2015 at 01:58:06PM -0700, Junio C Hamano wrote:
>
>> We forgot to terminate the payload given to write_file() with LF,
>> resulting in files that end with an incomplete line.  Teach the
>> wrappers builtin/am uses to make sure it adds LF at the end as
>> necessary.
>
> Is it even worth doing this step? It's completely reverted later in the
> series. I understand that we do not want to hold the fix to git-am
> hostage to write_file refactoring, but I don't see any reason these
> cannot all graduate as part of the same topic.
>
> Ignore me if you really are planning on doing the first two to "maint"
> and holding the others back for "master".

Not really. The primary reason why this step exists and 1-2/6 make a sufficient fix by themselves is because I wasn't even sure if 3-6/6 were worth doing.

As to "flags exposed to callers" vs "with and without gently", when we change the system to allow new modes of operations (e.g. somebody wants to write a binary file, or allocate more flag bits for their special case), I'd expect that we'd add a more general and verbose "write_file_with_options(path, flags, fmt, ...)"), gain experience with that function, and then possibly introduce canned thin wrappers (e.g. write_binary_file() that is a synonym to passing BINARY but not GENTLY) if the new thing proves widely useful, just like I left write_file() and write_file_gently() in as fairly common things to do.