Re: [PATCH] am: terminate state files with a newline

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 23, 2015, 19:05 UTC
Message-ID: <xmqqy4h16d1f.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150823055053.GA15849@yoshi.chippynet.com>

Paul Tan <pyokagan@gmail.com> writes:

> Did we ever explictly allow external programs to poke around the
> contents of the .git/rebase-apply directory?

We tell users to take a peek into it when "am" fails, don't we, by naming $GIT_DIR/rebase-apply/patch?

> -	write_file(am_path(state, "threeway"), 1, state->threeway ? "t" : "f");
> +	write_file(am_path(state, "threeway"), 1, "%s\n", state->threeway ? "t" : "f");

Stepping back a bit, after realizing that "write_file()" is a short-hand for "I have all information necessary to produce the full contents of a file, now go ahead and create and write that and close", I have to wonder what caller even wants to create a file with an incomplete line at the end.

All callers outside builtin/am.c except one caller uses it to produce a single line file. The oddball is "git branch" that uses it to prepare a temporary file used to edit branch description.

builtin/branch.c:	if (write_file(git_path(edit_description), 0, "%s", buf.buf)) {

The payload it prepares in buf.buf ends with a canned comment that ends with LF. So in that sense it is not even an oddball.

The above analysis makes me wonder if this is a simpler and more future proof approach.

Or did I miss any caller or a reasonable potential future use case that wants to create a binary file or a text file that ends with an incomplete line?

 wrapper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/wrapper.c b/wrapper.c
index e451463..7a92298 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -621,6 +621,13 @@ char *xgetcwd(void)
 	return strbuf_detach(&sb, NULL);
 }
 
+/*
+ * Create a TEXT file by specifying its full contents via fmt and the
+ * remainder of args that are used like "printf".  A terminating LF is
+ * added at the end of the file if it is missing (it is simpler for
+ * the callers because the function is often used to create a
+ * single-liner file).
+ */
 int write_file(const char *path, int fatal, const char *fmt, ...)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -634,6 +641,9 @@ int write_file(const char *path, int fatal, const char *fmt, ...)
 	va_start(params, fmt);
 	strbuf_vaddf(&sb, fmt, params);
 	va_end(params);
+	if (sb.len)
+		strbuf_complete_line(&sb);
+
 	if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
 		int err = errno;
 		close(fd);