[PATCH 0/3] make builtin-commit use commit_tree() and reduce_heads()

[PATCH 0/3] make builtin-commit use commit_tree() and reduce_heads()

[Miklos Vajna]

Hi,

On Tue, Sep 09, 2008 at 05:52:37PM +0100, Russell King <rmk@arm.linux.org.uk> wrote:
> Is this intentional, or is it a bug?

It is because builtin-commit does not use reduce_heads(), but
builtin-merge does.

Here are 3 patches to make builtin-commit use commit_tree() and
reduce_heads() as well.

The first one just improves the commit_tree() interface.

The second one is a cleanup, but it also adds reduce_heads() usage, to
fix the issue, pointed out by Russell King.

The third one adds new testcases, based on the ones provided by him.

Miklos Vajna (3):
  commit_tree(): add a new author parameter
  builtin-commit: use commit_tree()
  t7603: add new testcases to ensure builtin-commit uses reduce_heads()

 builtin-commit-tree.c         |    9 ++++--
 builtin-commit.c              |   63 +++++++++-------------------------------
 builtin-merge.c               |    4 +-
 builtin.h                     |    3 +-
 t/t7603-merge-reduce-heads.sh |   53 ++++++++++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 55 deletions(-)

[PATCH 1/3] commit_tree(): add a new author parameter

[Miklos Vajna]

In case it's NULL, it is still determined automatically, but now you
have the ability to specify one yourself.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-commit-tree.c |    9 ++++++---
 builtin-merge.c       |    4 ++--
 builtin.h             |    3 ++-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin-commit-tree.c b/builtin-commit-tree.c
index 8a5ba4c..33c0e82 100644
--- a/builtin-commit-tree.c
+++ b/builtin-commit-tree.c
@@ -46,7 +46,8 @@ static const char commit_utf8_warn[] =
 "variable i18n.commitencoding to the encoding your project uses.\n";
 
 int commit_tree(const char *msg, unsigned char *tree,
-		struct commit_list *parents, unsigned char *ret)
+		struct commit_list *parents, unsigned char *ret,
+		const char *author)
 {
 	int result;
 	int encoding_is_utf8;
@@ -74,7 +75,9 @@ int commit_tree(const char *msg, unsigned char *tree,
 	}
 
 	/* Person/date information */
-	strbuf_addf(&buffer, "author %s\n", git_author_info(IDENT_ERROR_ON_NO_NAME));
+	if (!author)
+		author = git_author_info(IDENT_ERROR_ON_NO_NAME);
+	strbuf_addf(&buffer, "author %s\n", author);
 	strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_ERROR_ON_NO_NAME));
 	if (!encoding_is_utf8)
 		strbuf_addf(&buffer, "encoding %s\n", git_commit_encoding);
@@ -123,7 +126,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 	if (strbuf_read(&buffer, 0, 0) < 0)
 		die("git commit-tree: read returned %s", strerror(errno));
 
-	if (!commit_tree(buffer.buf, tree_sha1, parents, commit_sha1)) {
+	if (!commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
 		printf("%s\n", sha1_to_hex(commit_sha1));
 		return 0;
 	}
diff --git a/builtin-merge.c b/builtin-merge.c
index 9ad9791..4a8ec60 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -691,7 +691,7 @@ static int merge_trivial(void)
 	parent->next = xmalloc(sizeof(struct commit_list *));
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
-	commit_tree(merge_msg.buf, result_tree, parent, result_commit);
+	commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
 	finish(result_commit, "In-index merge");
 	drop_save();
 	return 0;
@@ -720,7 +720,7 @@ static int finish_automerge(struct commit_list *common,
 	}
 	free_commit_list(remoteheads);
 	strbuf_addch(&merge_msg, '\n');
-	commit_tree(merge_msg.buf, result_tree, parents, result_commit);
+	commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
 	strbuf_addf(&buf, "Merge made by %s.", wt_strategy);
 	finish(result_commit, buf.buf);
 	strbuf_release(&buf);
diff --git a/builtin.h b/builtin.h
index e67cb20..8893b3c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -17,7 +17,8 @@ extern int read_line_with_nul(char *buf, int size, FILE *file);
 extern int fmt_merge_msg(int merge_summary, struct strbuf *in,
 	struct strbuf *out);
 extern int commit_tree(const char *msg, unsigned char *tree,
-		struct commit_list *parents, unsigned char *ret);
+		struct commit_list *parents, unsigned char *ret,
+		const char *author);
 extern int check_pager_config(const char *cmd);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
-- 
1.6.0.1

[PATCH 2/3] builtin-commit: use commit_tree()

[Miklos Vajna]

First, it adds less code than removes, second this allows us to use
recuce_heads() for parents, so that the parents of a merge will be
always the same with or without a conflict.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-commit.c |   63 ++++++++++++------------------------------------------
 1 files changed, 14 insertions(+), 49 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 8165bb3..ad055ea 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -670,11 +670,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
  * Find out if the message starting at position 'start' in the strbuf
  * contains only whitespace and Signed-off-by lines.
  */
-static int message_is_empty(struct strbuf *sb, int start)
+static int message_is_empty(struct strbuf *sb)
 {
 	struct strbuf tmpl;
 	const char *nl;
-	int eol, i;
+	int eol, i, start = 0;
 
 	if (cleanup_mode == CLEANUP_NONE && sb->len)
 		return 0;
@@ -929,34 +929,14 @@ static const char commit_utf8_warn[] =
 "You may want to amend it after fixing the message, or set the config\n"
 "variable i18n.commitencoding to the encoding your project uses.\n";
 
-static void add_parent(struct strbuf *sb, const unsigned char *sha1)
-{
-	struct object *obj = parse_object(sha1);
-	const char *parent = sha1_to_hex(sha1);
-	const char *cp;
-
-	if (!obj)
-		die("Unable to find commit parent %s", parent);
-	if (obj->type != OBJ_COMMIT)
-		die("Parent %s isn't a proper commit", parent);
-
-	for (cp = sb->buf; cp && (cp = strstr(cp, "\nparent ")); cp += 8) {
-		if (!memcmp(cp + 8, parent, 40) && cp[48] == '\n') {
-			error("duplicate parent %s ignored", parent);
-			return;
-		}
-	}
-	strbuf_addf(sb, "parent %s\n", parent);
-}
-
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
-	int header_len;
 	struct strbuf sb;
 	const char *index_file, *reflog_msg;
 	char *nl, *p;
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
+	struct commit_list *parents = NULL, **pptr = &parents;
 
 	git_config(git_commit_config, NULL);
 
@@ -971,13 +951,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		return 1;
 	}
 
-	/*
-	 * The commit object
-	 */
-	strbuf_init(&sb, 0);
-	strbuf_addf(&sb, "tree %s\n",
-		    sha1_to_hex(active_cache_tree->sha1));
-
 	/* Determine parents */
 	if (initial_commit) {
 		reflog_msg = "commit (initial)";
@@ -991,13 +964,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			die("could not parse HEAD commit");
 
 		for (c = commit->parents; c; c = c->next)
-			add_parent(&sb, c->item->object.sha1);
+			pptr = &commit_list_insert(c->item, pptr)->next;
 	} else if (in_merge) {
 		struct strbuf m;
 		FILE *fp;
 
 		reflog_msg = "commit (merge)";
-		add_parent(&sb, head_sha1);
+		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 		strbuf_init(&m, 0);
 		fp = fopen(git_path("MERGE_HEAD"), "r");
 		if (fp == NULL)
@@ -1007,24 +980,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			unsigned char sha1[20];
 			if (get_sha1_hex(m.buf, sha1) < 0)
 				die("Corrupt MERGE_HEAD file (%s)", m.buf);
-			add_parent(&sb, sha1);
+			pptr = &commit_list_insert(lookup_commit(sha1), pptr)->next;
 		}
 		fclose(fp);
 		strbuf_release(&m);
 	} else {
 		reflog_msg = "commit";
-		strbuf_addf(&sb, "parent %s\n", sha1_to_hex(head_sha1));
+		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
-
-	strbuf_addf(&sb, "author %s\n",
-		    fmt_ident(author_name, author_email, author_date, IDENT_ERROR_ON_NO_NAME));
-	strbuf_addf(&sb, "committer %s\n", git_committer_info(IDENT_ERROR_ON_NO_NAME));
-	if (!is_encoding_utf8(git_commit_encoding))
-		strbuf_addf(&sb, "encoding %s\n", git_commit_encoding);
-	strbuf_addch(&sb, '\n');
+	parents = reduce_heads(parents);
 
 	/* Finally, get the commit message */
-	header_len = sb.len;
+	strbuf_init(&sb, 0);
 	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
 		rollback_index_files();
 		die("could not read commit message");
@@ -1037,16 +1004,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
-	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
+	if (message_is_empty(&sb)) {
 		rollback_index_files();
 		fprintf(stderr, "Aborting commit due to empty commit message.\n");
 		exit(1);
 	}
-	strbuf_addch(&sb, '\0');
-	if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
-		fprintf(stderr, commit_utf8_warn);
 
-	if (write_sha1_file(sb.buf, sb.len - 1, commit_type, commit_sha1)) {
+	if (commit_tree(sb.buf, active_cache_tree->sha1, parents, commit_sha1,
+			fmt_ident(author_name, author_email, author_date,
+				IDENT_ERROR_ON_NO_NAME))) {
 		rollback_index_files();
 		die("failed to write commit object");
 	}
@@ -1055,12 +1021,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 					   initial_commit ? NULL : head_sha1,
 					   0);
 
-	nl = strchr(sb.buf + header_len, '\n');
+	nl = strchr(sb.buf, '\n');
 	if (nl)
 		strbuf_setlen(&sb, nl + 1 - sb.buf);
 	else
 		strbuf_addch(&sb, '\n');
-	strbuf_remove(&sb, 0, header_len);
 	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
 	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
 
-- 
1.6.0.1

[PATCH 3/3] t7603: add new testcases to ensure builtin-commit uses reduce_heads()

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 t/t7603-merge-reduce-heads.sh |   53 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index b47b7b9..7e17eb4 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -60,4 +60,57 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '
 	test -f c5.c
 '
 
+test_expect_success 'setup' '
+	for i in A B C D E
+	do
+		echo $i > $i.c &&
+		git add $i.c &&
+		git commit -m $i &&
+		git tag $i
+	done &&
+	git reset --hard A &&
+	for i in F G H I
+	do
+		echo $i > $i.c &&
+		git add $i.c &&
+		git commit -m $i &&
+		git tag $i
+	done
+'
+
+test_expect_success 'merge E and I' '
+	git reset --hard A &&
+	git merge E I
+'
+
+test_expect_success 'verify merge result' '
+	test $(git rev-parse HEAD^1) = $(git rev-parse E) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse I)
+'
+
+test_expect_success 'add conflicts' '
+	git reset --hard E &&
+	echo foo > file.c &&
+	git add file.c &&
+	git commit -m E2 &&
+	git tag E2 &&
+	git reset --hard I &&
+	echo bar >file.c &&
+	git add file.c &&
+	git commit -m I2 &&
+	git tag I2
+'
+
+test_expect_success 'merge E2 and I2, causing a conflict and resolve it' '
+	git reset --hard A &&
+	test_must_fail git merge E2 I2 &&
+	echo baz > file.c &&
+	git add file.c &&
+	git commit -m "resolve conflict"
+'
+
+test_expect_success 'verify merge result' '
+	test $(git rev-parse HEAD^1) = $(git rev-parse E2) &&
+	test $(git rev-parse HEAD^2) = $(git rev-parse I2)
+'
 test_done
-- 
1.6.0.1