Re: [PATCH v2 0/6] "am" state file fix with write_file() clean-up

From: Jeff King <peff@peff.net>
Date: Aug 25, 2015, 00:02 UTC
Message-ID: <20150825000231.GC13261@sigill.intra.peff.net>
In-Reply-To: <1440449890-29490-1-git-send-email-gitster@pobox.com>

On Mon, Aug 24, 2015 at 01:58:04PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> The workhorse helper function that implements "we have this (short)
> body of text; create a new file that contains it" has a "fatal"
> parameter, to which 1 was passed by almost all callers, but to
> casual readers, it was unclear what that 1 meant.  The patch [3/6]
> splits it to write_file() and write_file_gently() and drops this
> parameter that looks mysterious at the callsites.  A common helper
> function write_file_v() is introduced to implement these two as thin
> wrappers of it.

To be honest, I think the "flags" field is more maintainable going forward. Now you have _two_ functions, and any features you add to them have to go in both places. In 4/6 you add the WRITE_FILE_BINARY flag, but I notice that callers can't actually pass it. And adding it into write_file() would take us back to square-one with source compatibility.

> The patch [4/6] updates write_file_v() so that it does the "we are
> writing a text file.  Make sure it does not end with an incomplete
> line" logic that [2/6] added only to builtin/am.c, thusly reverting
> what was done to builtin/am.c in [2/6].

I notice this also converts "fatal" to "flags". It seemed weird to me that did not go into patch 3, but I guess it is OK (we know that write_file_v has no outstanding callers, since we just added it).

Show 6 quoted lines

> The patch [5/6] stops all callers that creates a single-liner file
> using write_file() and write_file_gently() from including the final
> LF to the format they pass.  This should not change the behaviour,
> but it probably makes it conceptually cleaner.  You have the contents
> to be placed on a single line, and the helper turns the contents
> into a proper "line".

Nice.

Show 7 quoted lines

> The patch [6/6] drops the final LF from the parameter to create a
> multi-line file; while this does not hurt in the sense that the
> callee will add a necessary LF back, I do not think it should be
> applied.  Conceptually, if you have a buffer that contains a bunch
> of lines and throw it at a helper to create a file, you'd better
> have the terminating LF yourself before asking the helper to put
> them in the file.

I agree we should drop this one.

-Peff