git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 12/14] revert: clarify label on conflict hunks

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Mar 21, 2010, 00:45 UTC
Message-ID
<20100321004521.GG23888@progeny.tock>
In-Reply-To
<20100321002535.GA23681@progeny.tock>

When reverting a commit, the commit being merged is not the commit to revert itself but its parent. Add “parent of” to the conflict hunk label to make this more clear.

The conflict hunk labels are all pieces of a single string written in the new get_message() function. Avoid some complication by using mempcpy to advance a pointer as the result is written.

Also free the corresponding temporary buffer (it was leaked before). This is not important because it is a small one-time allocation. It would become a memory leak if unnoticed when libifying revert.

This patch uses calls to strlen() instead of integer constants in some places. GCC will compute the length at compile time; I am not sure about other compilers, but this is not performance-critical anyway.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
From <http://thread.gmane.org/gmane.comp.version-control.git/142374/focus=142448>.
Thanks to Stephen for advice.
 builtin/revert.c                |  100 ++++++++++++++++++++++++---------------
 t/t3507-cherry-pick-conflict.sh |    4 +-
 2 files changed, 63 insertions(+), 41 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index eff5268..b314629 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -45,6 +45,8 @@ static const char *me;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+static char *get_encoding(const char *message);
+
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str =
@@ -73,33 +75,63 @@ static void parse_args(int argc, const char **argv)
 		exit(1);
 }
 
-static char *get_oneline(const char *message)
+struct commit_message {
+	char *parent_label;
+	const char *label;
+	const char *subject;
+	char *reencoded_message;
+	const char *message;
+};
+
+static int get_message(const char *raw_message, struct commit_message *out)
 {
-	char *result;
-	const char *p = message, *abbrev, *eol;
+	const char *encoding;
+	const char *p, *abbrev, *eol;
+	char *q;
 	int abbrev_len, oneline_len;
 
-	if (!p)
-		die ("Could not read commit message of %s",
-				sha1_to_hex(commit->object.sha1));
+	if (!raw_message)
+		return -1;
+	encoding = get_encoding(raw_message);
+	if (!encoding)
+		encoding = "UTF-8";
+	if (!git_commit_encoding)
+		git_commit_encoding = "UTF-8";
+	if ((out->reencoded_message = reencode_string(raw_message,
+					git_commit_encoding, encoding)))
+		out->message = out->reencoded_message;
+
+	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+	abbrev_len = strlen(abbrev);
+
+	/* Find beginning and end of commit subject. */
+	p = out->message;
 	while (*p && (*p != '\n' || p[1] != '\n'))
 		p++;
-
 	if (*p) {
 		p += 2;
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
 	} else
 		eol = p;
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
 	oneline_len = eol - p;
-	result = xmalloc(abbrev_len + 5 + oneline_len);
-	memcpy(result, abbrev, abbrev_len);
-	memcpy(result + abbrev_len, "... ", 4);
-	memcpy(result + abbrev_len + 4, p, oneline_len);
-	result[abbrev_len + 4 + oneline_len] = '\0';
-	return result;
+
+	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+			      strlen("... ") + oneline_len + 1);
+	q = out->parent_label;
+	q = mempcpy(q, "parent of ", strlen("parent of "));
+	out->label = q;
+	q = mempcpy(q, abbrev, abbrev_len);
+	q = mempcpy(q, "... ", strlen("... "));
+	out->subject = q;
+	q = mempcpy(q, p, oneline_len);
+	*q = '\0';
+	return 0;
+}
+
+static void free_message(struct commit_message *msg) {
+	free(msg->parent_label);
+	free(msg->reencoded_message);
 }
 
 static char *get_encoding(const char *message)
@@ -248,9 +280,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
+	const char *next_label;
 	int i, index_fd, clean;
-	char *oneline, *reencoded_message = NULL;
-	const char *message, *encoding;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+
 	char *defmsg = git_pathdup("MERGE_MSG");
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
@@ -314,14 +347,14 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	else
 		parent = commit->parents->item;
 
-	if (!(message = commit->buffer))
-		die ("Cannot get commit message for %s",
-				sha1_to_hex(commit->object.sha1));
-
 	if (parent && parse_commit(parent) < 0)
 		die("%s: cannot parse parent commit %s",
 		    me, sha1_to_hex(parent->object.sha1));
 
+	if (get_message(commit->buffer, &msg) != 0)
+		die ("Cannot get commit message for %s",
+				sha1_to_hex(commit->object.sha1));
+
 	/*
 	 * "commit" is an existing commit.  We would want to apply
 	 * the difference it introduces since its first parent "prev"
@@ -332,24 +365,12 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
 					   LOCK_DIE_ON_ERROR);
 
-	encoding = get_encoding(message);
-	if (!encoding)
-		encoding = "UTF-8";
-	if (!git_commit_encoding)
-		git_commit_encoding = "UTF-8";
-	if ((reencoded_message = reencode_string(message,
-					git_commit_encoding, encoding)))
-		message = reencoded_message;
-
-	oneline = get_oneline(message);
-
 	if (action == REVERT) {
-		char *oneline_body = strchr(oneline, ' ');
-
 		base = commit;
 		next = parent;
+		next_label = msg.parent_label;
 		add_to_msg("Revert \"");
-		add_to_msg(oneline_body + 1);
+		add_to_msg(msg.subject);
 		add_to_msg("\"\n\nThis reverts commit ");
 		add_to_msg(sha1_to_hex(commit->object.sha1));
 
@@ -361,8 +382,9 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	} else {
 		base = parent;
 		next = commit;
-		set_author_ident_env(message);
-		add_message_to_msg(message);
+		next_label = msg.label;
+		set_author_ident_env(msg.message);
+		add_message_to_msg(msg.message);
 		if (no_replay) {
 			add_to_msg("(cherry picked from commit ");
 			add_to_msg(sha1_to_hex(commit->object.sha1));
@@ -373,7 +395,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	read_cache();
 	init_merge_options(&o);
 	o.branch1 = "HEAD";
-	o.branch2 = oneline;
+	o.branch2 = next ? next_label : "(empty tree)";
 
 	head_tree = parse_tree_indirect(head);
 	next_tree = next ? next->tree : empty_tree();
@@ -437,7 +459,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		args[i] = NULL;
 		return execv_git_cmd(args);
 	}
-	free(reencoded_message);
+	free_message(&msg);
 	free(defmsg);
 
 	return 0;
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index e856356..6a20817 100644
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -138,7 +138,7 @@ test_expect_success 'revert also handles conflicts sanely' '
 	a
 	=======
 	b
-	>>>>>>> objid picked
+	>>>>>>> parent of objid picked
 	EOF
 	{
 		git checkout picked -- foo &&
@@ -183,7 +183,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	c
 	=======
 	b
-	>>>>>>> objid picked
+	>>>>>>> parent of objid picked
 	EOF
 
 	git update-index --refresh &&

-- 
1.7.0.2
Previous: Jonathan NiederNext: Jonathan Nieder
Message 14 of 16 in “What's cooking in git.git (Mar 2010, #05; Sat, 20)”
  1. Junio C HamanoMar 20, 2010
  2. 0/14 jn/merge-diff3-labelJonathan Nieder, Mar 21, 2010
  3. 01/14 tests: document format of conflicts from checkout -mJonathan Nieder, Mar 21, 2010
  4. 02/14 tests: document cherry-pick behavior in face of conflictsJonathan Nieder, Mar 21, 2010
  5. 03/14 xdl_merge(): add optional ancestor label to diff3-style outputJonathan Nieder, Mar 21, 2010
  6. 04/14 xdl_merge(): move file1 and file2 labels to xmparam structureJonathan Nieder, Mar 21, 2010
  7. 05/14 merge-file --diff3: add a label for ancestorJonathan Nieder, Mar 21, 2010
  8. 06/14 ll_merge(): add ancestor label parameter for diff3-style outputJonathan Nieder, Mar 21, 2010
  9. 07/14 checkout --conflict=diff3: add a label for ancestorJonathan Nieder, Mar 21, 2010
  10. 08/14 merge_file(): add comment explaining behavior wrt conflict styleJonathan Nieder, Mar 21, 2010
  11. 09/14 merge_trees(): add ancestor label parameter for diff3-style outputJonathan Nieder, Mar 21, 2010
  12. 10/14 checkout -m --conflict=diff3: add a label for ancestorJonathan Nieder, Mar 21, 2010
  13. 11/14 compat: add mempcpy()Jonathan Nieder, Mar 21, 2010
  14. 12/14 revert: clarify label on conflict hunksJonathan Nieder, Mar 21, 2010
  15. 13/14 cherry-pick, revert: add a label for ancestorJonathan Nieder, Mar 21, 2010
  16. 14/14 merge-recursive: add a label for ancestorJonathan Nieder, Mar 21, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.