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

From: Jeff King <peff@peff.net>
Date: Aug 25, 2015, 16:47 UTC
Message-ID: <20150825164701.GA10060@sigill.intra.peff.net>
In-Reply-To: <xmqqtwrn1gu6.fsf@gitster.dls.corp.google.com>

On Tue, Aug 25, 2015 at 09:19:13AM -0700, Junio C Hamano wrote:

Show 10 quoted lines

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

Yeah, that works. It is a bit of a gamble to me. If we never add a lot more options, the end result is much nicer (callers do not deal with the flag option at all). But if we do, we end up with the mess that get_sha1_with_* and add_pending_object() got into.

One can always refactor later, too. In that sense, the BINARY flag is not useful (nobody uses it, and we do not plan to do so). We could just make write_file_v unconditionally complete lines[1].

But I'm OK with what you posted, as well. I think this interface is not worth spending a lot of time micro-nit-picking.

-Peff

[1] In fact, I'd be surprised if this function works well for non-text
    data anyway, as it relies on printf-style formatting. You cannot use
    it to write a string with embedded NULs, for example.

    If we wanted to support that case, we would probably break out:

        int write_buf_to_file(const char *filename,
	                      const char *buf, size_t len);

    as a thin wrapper for open/write_in_full/close. And then write_to_file()
    would format into a strbuf, complete a newline, and pass the result
    to it.