[PATCH] am: terminate state files with a newline

[Paul Tan <pyokagan@gmail.com>]

On Thu, Aug 20, 2015 at 11:40:20AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > The format of the files '.git/rebase-apply/{next,last}' changed slightly
> > with the recent builtin 'git am' conversion: while these files were
> > newline-terminated when written by the scripted version, the ones written
> > by the builtin are not.
> 
> Thanks for noticing; that should be corrected, I think.

Okay then, this patch should correct this.

Did we ever explictly allow external programs to poke around the
contents of the .git/rebase-apply directory? I think it may not be so
good, as it means that it may not be possible to switch the storage
format in the future (e.g. to allow atomic modifications, maybe?) :-/ .

Regards,
Paul

-- >8 --
Subject: [PATCH] am: terminate state files with a newline

Since builtin/am.c replaced git-am.sh in 783d7e8 (builtin-am: remove
redirection to git-am.sh, 2015-08-04), the state files written by git-am
did not terminate with a newline.

This is because the code in builtin/am.c did not write the newline to
the state files.

While the git codebase has no problems with the missing newline,
external software which read the contents of the state directory may be
strict about the existence of the terminating newline, and would thus
break.

Fix this by correcting the relevant calls to write_file() to ensure that
the state files written terminate with a newline, matching how git-am.sh
behaves.

While we are fixing the write_file() calls, fix the writing of the
"dirtyindex" file as well -- we should be creating an empty file to
match the behavior of git-am.sh.

Reported-by: SZEDER Gábor <szeder@ira.uka.de>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1399c8d..2e57fad 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -994,13 +994,13 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	if (state->rebasing)
 		state->threeway = 1;
 
-	write_file(am_path(state, "threeway"), 1, state->threeway ? "t" : "f");
+	write_file(am_path(state, "threeway"), 1, "%s\n", state->threeway ? "t" : "f");
 
-	write_file(am_path(state, "quiet"), 1, state->quiet ? "t" : "f");
+	write_file(am_path(state, "quiet"), 1, "%s\n", state->quiet ? "t" : "f");
 
-	write_file(am_path(state, "sign"), 1, state->signoff ? "t" : "f");
+	write_file(am_path(state, "sign"), 1, "%s\n", state->signoff ? "t" : "f");
 
-	write_file(am_path(state, "utf8"), 1, state->utf8 ? "t" : "f");
+	write_file(am_path(state, "utf8"), 1, "%s\n", state->utf8 ? "t" : "f");
 
 	switch (state->keep) {
 	case KEEP_FALSE:
@@ -1016,9 +1016,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 		die("BUG: invalid value for state->keep");
 	}
 
-	write_file(am_path(state, "keep"), 1, "%s", str);
+	write_file(am_path(state, "keep"), 1, "%s\n", str);
 
-	write_file(am_path(state, "messageid"), 1, state->message_id ? "t" : "f");
+	write_file(am_path(state, "messageid"), 1, "%s\n", state->message_id ? "t" : "f");
 
 	switch (state->scissors) {
 	case SCISSORS_UNSET:
@@ -1034,10 +1034,10 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 		die("BUG: invalid value for state->scissors");
 	}
 
-	write_file(am_path(state, "scissors"), 1, "%s", str);
+	write_file(am_path(state, "scissors"), 1, "%s\n", str);
 
 	sq_quote_argv(&sb, state->git_apply_opts.argv, 0);
-	write_file(am_path(state, "apply-opt"), 1, "%s", sb.buf);
+	write_file(am_path(state, "apply-opt"), 1, "%s\n", sb.buf);
 
 	if (state->rebasing)
 		write_file(am_path(state, "rebasing"), 1, "%s", "");
@@ -1045,7 +1045,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 		write_file(am_path(state, "applying"), 1, "%s", "");
 
 	if (!get_sha1("HEAD", curr_head)) {
-		write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(curr_head));
+		write_file(am_path(state, "abort-safety"), 1, "%s\n", sha1_to_hex(curr_head));
 		if (!state->rebasing)
 			update_ref("am", "ORIG_HEAD", curr_head, NULL, 0,
 					UPDATE_REFS_DIE_ON_ERR);
@@ -1060,9 +1060,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
 	 * session is in progress, they should be written last.
 	 */
 
-	write_file(am_path(state, "next"), 1, "%d", state->cur);
+	write_file(am_path(state, "next"), 1, "%d\n", state->cur);
 
-	write_file(am_path(state, "last"), 1, "%d", state->last);
+	write_file(am_path(state, "last"), 1, "%d\n", state->last);
 
 	strbuf_release(&sb);
 }
@@ -1095,12 +1095,12 @@ static void am_next(struct am_state *state)
 	unlink(am_path(state, "original-commit"));
 
 	if (!get_sha1("HEAD", head))
-		write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(head));
+		write_file(am_path(state, "abort-safety"), 1, "%s\n", sha1_to_hex(head));
 	else
 		write_file(am_path(state, "abort-safety"), 1, "%s", "");
 
 	state->cur++;
-	write_file(am_path(state, "next"), 1, "%d", state->cur);
+	write_file(am_path(state, "next"), 1, "%d\n", state->cur);
 }
 
 /**
@@ -1461,7 +1461,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
 	write_commit_patch(state, commit);
 
 	hashcpy(state->orig_commit, commit_sha1);
-	write_file(am_path(state, "original-commit"), 1, "%s",
+	write_file(am_path(state, "original-commit"), 1, "%s\n",
 			sha1_to_hex(commit_sha1));
 
 	return 0;
@@ -1764,7 +1764,7 @@ static void am_run(struct am_state *state, int resume)
 	refresh_and_write_cache();
 
 	if (index_has_changes(&sb)) {
-		write_file(am_path(state, "dirtyindex"), 1, "t");
+		write_file(am_path(state, "dirtyindex"), 1, "%s", "");
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
 
-- 
2.5.0.400.gff86faf.dirty