Merge pull request #5275 from pks-t/pks/reflogs-with-newlines

reflogs: fix behaviour around reflogs with newlines

src/parse.c

@@ -101,6 +101,16 @@ int git_parse_advance_digit(int64_t *out, git_parse_ctx *ctx, int base)
 	return 0;
 }
 
+int git_parse_advance_oid(git_oid *out, git_parse_ctx *ctx)
+{
+	if (ctx->line_len < GIT_OID_HEXSZ)
+		return -1;
+	if ((git_oid_fromstrn(out, ctx->line, GIT_OID_HEXSZ)) < 0)
+		return -1;
+	git_parse_advance_chars(ctx, GIT_OID_HEXSZ);
+	return 0;
+}
+
 int git_parse_peek(char *out, git_parse_ctx *ctx, int flags)
 {
 	size_t remain = ctx->line_len;

src/parse.h

@@ -50,6 +50,7 @@ int git_parse_advance_expected(
 int git_parse_advance_ws(git_parse_ctx *ctx);
 int git_parse_advance_nl(git_parse_ctx *ctx);
 int git_parse_advance_digit(int64_t *out, git_parse_ctx *ctx, int base);
+int git_parse_advance_oid(git_oid *out, git_parse_ctx *ctx);
 
 enum GIT_PARSE_PEEK_FLAGS {
 	GIT_PARSE_PEEK_SKIP_WHITESPACE = (1 << 0)

src/refdb_fs.c

@@ -13,6 +13,7 @@
 #include "futils.h"
 #include "filebuf.h"
 #include "pack.h"
+#include "parse.h"
 #include "reflog.h"
 #include "refdb.h"
 #include "iterator.h"
@@ -1651,70 +1652,57 @@ static int reflog_alloc(git_reflog **reflog, const char *name)
 
 static int reflog_parse(git_reflog *log, const char *buf, size_t buf_size)
 {
-	const char *ptr;
-	git_reflog_entry *entry;
-
-#define seek_forward(_increase) do { \
-	if (_increase >= buf_size) { \
-		git_error_set(GIT_ERROR_INVALID, "ran out of data while parsing reflog"); \
-		goto fail; \
-	} \
-	buf += _increase; \
-	buf_size -= _increase; \
-	} while (0)
-
-	while (buf_size > GIT_REFLOG_SIZE_MIN) {
-		entry = git__calloc(1, sizeof(git_reflog_entry));
-		GIT_ERROR_CHECK_ALLOC(entry);
+	git_parse_ctx parser = GIT_PARSE_CTX_INIT;
 
-		entry->committer = git__calloc(1, sizeof(git_signature));
-		GIT_ERROR_CHECK_ALLOC(entry->committer);
+	if ((git_parse_ctx_init(&parser, buf, buf_size)) < 0)
+		return -1;
 
-		if (git_oid_fromstrn(&entry->oid_old, buf, GIT_OID_HEXSZ) < 0)
-			goto fail;
-		seek_forward(GIT_OID_HEXSZ + 1);
+	for (; parser.remain_len; git_parse_advance_line(&parser)) {
+		git_reflog_entry *entry;
+		const char *sig;
+		char c;
 
-		if (git_oid_fromstrn(&entry->oid_cur, buf, GIT_OID_HEXSZ) < 0)
-			goto fail;
-		seek_forward(GIT_OID_HEXSZ + 1);
+		entry = git__calloc(1, sizeof(*entry));
+		GIT_ERROR_CHECK_ALLOC(entry);
+		entry->committer = git__calloc(1, sizeof(*entry->committer));
+		GIT_ERROR_CHECK_ALLOC(entry->committer);
 
-		ptr = buf;
+		if (git_parse_advance_oid(&entry->oid_old, &parser) < 0 ||
+		    git_parse_advance_expected(&parser, " ", 1) < 0 ||
+		    git_parse_advance_oid(&entry->oid_cur, &parser) < 0)
+			goto next;
 
-		/* Seek forward to the end of the signature. */
-		while (*buf && *buf != '\t' && *buf != '\n')
-			seek_forward(1);
+		sig = parser.line;
+		while (git_parse_peek(&c, &parser, 0) == 0 && c != '\t' && c != '\n')
+			git_parse_advance_chars(&parser, 1);
 
-		if (git_signature__parse(entry->committer, &ptr, buf + 1, NULL, *buf) < 0)
-			goto fail;
+		if (git_signature__parse(entry->committer, &sig, parser.line, NULL, 0) < 0)
+			goto next;
 
-		if (*buf == '\t') {
-			/* We got a message. Read everything till we reach LF. */
-			seek_forward(1);
-			ptr = buf;
+		if (c == '\t') {
+			size_t len;
+			git_parse_advance_chars(&parser, 1);
 
-			while (*buf && *buf != '\n')
-				seek_forward(1);
+			len = parser.line_len;
+			if (parser.line[len - 1] == '\n')
+				len--;
 
-			entry->msg = git__strndup(ptr, buf - ptr);
+			entry->msg = git__strndup(parser.line, len);
 			GIT_ERROR_CHECK_ALLOC(entry->msg);
-		} else
-			entry->msg = NULL;
+		}
 
-		while (*buf && *buf == '\n' && buf_size > 1)
-			seek_forward(1);
+		if ((git_vector_insert(&log->entries, entry)) < 0) {
+			git_reflog_entry__free(entry);
+			return -1;
+		}
 
-		if (git_vector_insert(&log->entries, entry) < 0)
-			goto fail;
+		continue;
+
+next:
+		git_reflog_entry__free(entry);
 	}
 
 	return 0;
-
-#undef seek_forward
-
-fail:
-	git_reflog_entry__free(entry);
-
-	return -1;
 }
 
 static int create_new_reflog_file(const char *filepath)
@@ -1856,8 +1844,15 @@ static int serialize_reflog_entry(
 	git_buf_rtrim(buf);
 
 	if (msg) {
+		size_t i;
+
 		git_buf_putc(buf, '\t');
 		git_buf_puts(buf, msg);
+
+		for (i = 0; i < buf->size - 2; i++)
+			if (buf->ptr[i] == '\n')
+				buf->ptr[i] = ' ';
+		git_buf_rtrim(buf);
 	}
 
 	git_buf_putc(buf, '\n');

src/reflog.c

@@ -74,9 +74,8 @@ int git_reflog_write(git_reflog *reflog)
 
 int git_reflog_append(git_reflog *reflog, const git_oid *new_oid, const git_signature *committer, const char *msg)
 {
-	git_reflog_entry *entry;
 	const git_reflog_entry *previous;
-	const char *newline;
+	git_reflog_entry *entry;
 
 	assert(reflog && new_oid && committer);
 
@@ -87,19 +86,18 @@ int git_reflog_append(git_reflog *reflog, const git_oid *new_oid, const git_sign
 		goto cleanup;
 
 	if (msg != NULL) {
-		if ((entry->msg = git__strdup(msg)) == NULL)
-			goto cleanup;
+		size_t i, msglen = strlen(msg);
 
-		newline = strchr(msg, '\n');
-
-		if (newline) {
-			if (newline[1] != '\0') {
-				git_error_set(GIT_ERROR_INVALID, "reflog message cannot contain newline");
-				goto cleanup;
-			}
+		if ((entry->msg = git__strndup(msg, msglen)) == NULL)
+			goto cleanup;
 
-			entry->msg[newline - msg] = '\0';
-		}
+		/*
+		 * Replace all newlines with spaces, except for
+		 * the final trailing newline.
+		 */
+		for (i = 0; i < msglen; i++)
+			if (entry->msg[i] == '\n')
+				entry->msg[i] = ' ';
 	}
 
 	previous = git_reflog_entry_byindex(reflog, 0);

src/stash.c

@@ -437,36 +437,33 @@ cleanup:
 	return error;
 }
 
-static int prepare_worktree_commit_message(
-	git_buf* msg,
-	const char *user_message)
+static int prepare_worktree_commit_message(git_buf *out, const char *user_message)
 {
 	git_buf buf = GIT_BUF_INIT;
-	int error;
-
-	if ((error = git_buf_set(&buf, git_buf_cstr(msg), git_buf_len(msg))) < 0)
-		return error;
-
-	git_buf_clear(msg);
+	int error = 0;
 
-	if (!user_message)
-		git_buf_printf(msg, "WIP on %s", git_buf_cstr(&buf));
-	else {
+	if (!user_message) {
+		git_buf_printf(&buf, "WIP on %s", git_buf_cstr(out));
+	} else {
 		const char *colon;
 
-		if ((colon = strchr(git_buf_cstr(&buf), ':')) == NULL)
+		if ((colon = strchr(git_buf_cstr(out), ':')) == NULL)
 			goto cleanup;
 
-		git_buf_puts(msg, "On ");
-		git_buf_put(msg, git_buf_cstr(&buf), colon - buf.ptr);
-		git_buf_printf(msg, ": %s\n", user_message);
+		git_buf_puts(&buf, "On ");
+		git_buf_put(&buf, git_buf_cstr(out), colon - out->ptr);
+		git_buf_printf(&buf, ": %s\n", user_message);
+	}
+
+	if (git_buf_oom(&buf)) {
+		error = -1;
+		goto cleanup;
 	}
 
-	error = (git_buf_oom(msg) || git_buf_oom(&buf)) ? -1 : 0;
+	git_buf_swap(out, &buf);
 
 cleanup:
 	git_buf_dispose(&buf);
-
 	return error;
 }
 
@@ -497,10 +494,7 @@ static int is_dirty_cb(const char *path, unsigned int status, void *payload)
 	return GIT_PASSTHROUGH;
 }
 
-static int ensure_there_are_changes_to_stash(
-	git_repository *repo,
-	bool include_untracked_files,
-	bool include_ignored_files)
+static int ensure_there_are_changes_to_stash(git_repository *repo, uint32_t flags)
 {
 	int error;
 	git_status_options opts = GIT_STATUS_OPTIONS_INIT;
@@ -508,11 +502,11 @@ static int ensure_there_are_changes_to_stash(
 	opts.show  = GIT_STATUS_SHOW_INDEX_AND_WORKDIR;
 	opts.flags = GIT_STATUS_OPT_EXCLUDE_SUBMODULES;
 
-	if (include_untracked_files)
+	if (flags & GIT_STASH_INCLUDE_UNTRACKED)
 		opts.flags |= GIT_STATUS_OPT_INCLUDE_UNTRACKED |
 			GIT_STATUS_OPT_RECURSE_UNTRACKED_DIRS;
 
-	if (include_ignored_files)
+	if (flags & GIT_STASH_INCLUDE_IGNORED)
 		opts.flags |= GIT_STATUS_OPT_INCLUDE_IGNORED |
 			GIT_STATUS_OPT_RECURSE_IGNORED_DIRS;
 
@@ -527,20 +521,14 @@ static int ensure_there_are_changes_to_stash(
 	return error;
 }
 
-static int reset_index_and_workdir(
-	git_repository *repo,
-	git_commit *commit,
-	bool remove_untracked,
-	bool remove_ignored)
+static int reset_index_and_workdir(git_repository *repo, git_commit *commit, uint32_t flags)
 {
 	git_checkout_options opts = GIT_CHECKOUT_OPTIONS_INIT;
 
 	opts.checkout_strategy = GIT_CHECKOUT_FORCE;
-
-	if (remove_untracked)
+	if (flags & GIT_STASH_INCLUDE_UNTRACKED)
 		opts.checkout_strategy |= GIT_CHECKOUT_REMOVE_UNTRACKED;
-
-	if (remove_ignored)
+	if (flags & GIT_STASH_INCLUDE_IGNORED)
 		opts.checkout_strategy |= GIT_CHECKOUT_REMOVE_IGNORED;
 
 	return git_checkout_tree(repo, (git_object *)commit, &opts);
@@ -566,31 +554,26 @@ int git_stash_save(
 	if ((error = retrieve_base_commit_and_message(&b_commit, &msg, repo)) < 0)
 		goto cleanup;
 
-	if ((error = ensure_there_are_changes_to_stash(
-		repo,
-		(flags & GIT_STASH_INCLUDE_UNTRACKED) != 0,
-		(flags & GIT_STASH_INCLUDE_IGNORED) != 0)) < 0)
+	if ((error = ensure_there_are_changes_to_stash(repo, flags)) < 0)
 		goto cleanup;
 
 	if ((error = git_repository_index(&index, repo)) < 0)
 		goto cleanup;
 
-	if ((error = commit_index(
-			&i_commit, repo, index, stasher, git_buf_cstr(&msg), b_commit)) < 0)
+	if ((error = commit_index(&i_commit, repo, index, stasher,
+				  git_buf_cstr(&msg), b_commit)) < 0)
 		goto cleanup;
 
 	if ((flags & (GIT_STASH_INCLUDE_UNTRACKED | GIT_STASH_INCLUDE_IGNORED)) &&
-		(error = commit_untracked(
-			&u_commit, repo, stasher, git_buf_cstr(&msg),
-			i_commit, flags)) < 0)
+	    (error = commit_untracked(&u_commit, repo, stasher,
+				      git_buf_cstr(&msg), i_commit, flags)) < 0)
 		goto cleanup;
 
 	if ((error = prepare_worktree_commit_message(&msg, message)) < 0)
 		goto cleanup;
 
-	if ((error = commit_worktree(
-			out, repo, stasher, git_buf_cstr(&msg),
-			i_commit, b_commit, u_commit)) < 0)
+	if ((error = commit_worktree(out, repo, stasher, git_buf_cstr(&msg),
+				     i_commit, b_commit, u_commit)) < 0)
 		goto cleanup;
 
 	git_buf_rtrim(&msg);
@@ -598,11 +581,8 @@ int git_stash_save(
 	if ((error = update_reflog(out, repo, git_buf_cstr(&msg))) < 0)
 		goto cleanup;
 
-	if ((error = reset_index_and_workdir(
-		repo,
-		((flags & GIT_STASH_KEEP_INDEX) != 0) ? i_commit : b_commit,
-		(flags & GIT_STASH_INCLUDE_UNTRACKED) != 0,
-		(flags & GIT_STASH_INCLUDE_IGNORED) != 0)) < 0)
+	if ((error = reset_index_and_workdir(repo, (flags & GIT_STASH_KEEP_INDEX) ? i_commit : b_commit,
+					     flags)) < 0)
 		goto cleanup;
 
 cleanup:

tests/refs/reflog/messages.c

@@ -281,6 +281,27 @@ void test_refs_reflog_messages__creating_a_direct_reference(void)
 	git_reference_free(reference);
 }
 
+void test_refs_reflog_messages__newline_gets_replaced(void)
+{
+	const git_reflog_entry *entry;
+	git_signature *signature;
+	git_reflog *reflog;
+	git_oid oid;
+
+	cl_git_pass(git_signature_now(&signature, "me", "foo@example.com"));
+	cl_git_pass(git_oid_fromstr(&oid, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"));
+
+	cl_git_pass(git_reflog_read(&reflog, g_repo, "HEAD"));
+	cl_assert_equal_sz(7, git_reflog_entrycount(reflog));
+	cl_git_pass(git_reflog_append(reflog, &oid, signature, "inner\nnewline"));
+	cl_assert_equal_sz(8, git_reflog_entrycount(reflog));
+
+	cl_assert(entry = git_reflog_entry_byindex(reflog, 0));
+	cl_assert_equal_s(git_reflog_entry_message(entry), "inner newline");
+
+	git_signature_free(signature);
+	git_reflog_free(reflog);
+}
 
 void test_refs_reflog_messages__renaming_ref(void)
 {

tests/refs/reflog/reflog.c

@@ -87,15 +87,13 @@ void test_refs_reflog_reflog__append_then_read(void)
 	cl_git_pass(git_signature_now(&committer, "foo", "foo@bar"));
 
 	cl_git_pass(git_reflog_read(&reflog, g_repo, new_ref));
-
-	cl_git_fail(git_reflog_append(reflog, &oid, committer, "no inner\nnewline"));
 	cl_git_pass(git_reflog_append(reflog, &oid, committer, NULL));
 	cl_git_pass(git_reflog_append(reflog, &oid, committer, commit_msg "\n"));
 	cl_git_pass(git_reflog_write(reflog));
-	git_reflog_free(reflog);
 
 	assert_appends(committer, &oid);
 
+	git_reflog_free(reflog);
 	git_signature_free(committer);
 }
 
@@ -224,45 +222,45 @@ void test_refs_reflog_reflog__reading_the_reflog_from_a_reference_with_no_log_re
 	git_buf_dispose(&subtrees_log_path);
 }
 
-void test_refs_reflog_reflog__reading_a_reflog_with_invalid_format_returns_error(void)
+void test_refs_reflog_reflog__reading_a_reflog_with_invalid_format_succeeds(void)
 {
 	git_reflog *reflog;
-	const git_error *error;
 	const char *refname = "refs/heads/newline";
 	const char *refmessage =
 		"Reflog*message with a newline and enough content after it to pass the GIT_REFLOG_SIZE_MIN check inside reflog_parse.";
+	const git_reflog_entry *entry;
 	git_reference *ref;
 	git_oid id;
 	git_buf logpath = GIT_BUF_INIT, logcontents = GIT_BUF_INIT;
 	char *star;
 
-	git_oid_fromstr(&id, current_master_tip);
-
-	/* create a new branch */
+	/* Create a new branch. */
+	cl_git_pass(git_oid_fromstr(&id, current_master_tip));
 	cl_git_pass(git_reference_create(&ref, g_repo, refname, &id, 1, refmessage));
 
-	/* corrupt the branch reflog by introducing a newline inside the reflog message (we replace '*' with '\n') */
-	git_buf_join_n(&logpath, '/', 3, git_repository_path(g_repo), GIT_REFLOG_DIR, refname);
+	/*
+	 * Corrupt the branch reflog by introducing a newline inside the reflog message.
+	 * We do this by replacing '*' with '\n'
+	 */
+	cl_git_pass(git_buf_join_n(&logpath, '/', 3, git_repository_path(g_repo), GIT_REFLOG_DIR, refname));
 	cl_git_pass(git_futils_readbuffer(&logcontents, git_buf_cstr(&logpath)));
 	cl_assert((star = strchr(git_buf_cstr(&logcontents), '*')) != NULL);
 	*star = '\n';
 	cl_git_rewritefile(git_buf_cstr(&logpath), git_buf_cstr(&logcontents));
 
-	/* confirm that the file was rewritten successfully and now contains a '\n' in the expected location */
+	/*
+	 * Confirm that the file was rewritten successfully
+	 * and now contains a '\n' in the expected location
+	 */
 	cl_git_pass(git_futils_readbuffer(&logcontents, git_buf_cstr(&logpath)));
 	cl_assert(strstr(git_buf_cstr(&logcontents), "Reflog\nmessage") != NULL);
 
-	/* clear the error state so we can capture the error generated by git_reflog_read */
-	git_error_clear();
-
-	cl_git_fail(git_reflog_read(&reflog, g_repo, refname));
-
-	error = git_error_last();
-
-	cl_assert(error != NULL);
-	cl_assert_equal_s("unable to parse OID - contains invalid characters", error->message);
+	cl_git_pass(git_reflog_read(&reflog, g_repo, refname));
+	cl_assert(entry = git_reflog_entry_byindex(reflog, 0));
+	cl_assert_equal_s(git_reflog_entry_message(entry), "Reflog");
 
 	git_reference_free(ref);
+	git_reflog_free(reflog);
 	git_buf_dispose(&logpath);
 	git_buf_dispose(&logcontents);
 }

tests/stash/save.c

@@ -283,6 +283,26 @@ void test_stash_save__stashing_updates_the_reflog(void)
 	assert_object_oid("refs/stash@{1}", NULL, GIT_OBJECT_COMMIT);
 }
 
+void test_stash_save__multiline_message(void)
+{
+	const char *msg = "This\n\nis a multiline message\n";
+	const git_reflog_entry *entry;
+	git_reflog *reflog;
+
+	assert_object_oid("refs/stash@{0}", NULL, GIT_OBJECT_COMMIT);
+
+	cl_git_pass(git_stash_save(&stash_tip_oid, repo, signature, msg, GIT_STASH_DEFAULT));
+
+	cl_git_pass(git_reflog_read(&reflog, repo, "refs/stash"));
+	cl_assert(entry = git_reflog_entry_byindex(reflog, 0));
+	cl_assert_equal_s(git_reflog_entry_message(entry), "On master: This  is a multiline message");
+
+	assert_object_oid("refs/stash@{0}", git_oid_tostr_s(&stash_tip_oid), GIT_OBJECT_COMMIT);
+	assert_commit_message_contains("refs/stash@{0}", msg);
+
+	git_reflog_free(reflog);
+}
+
 void test_stash_save__cannot_stash_when_there_are_no_local_change(void)
 {
 	git_index *index;