[PATCH] commit: simplify building parents list

[PATCH] commit: simplify building parents list

[René Scharfe]

Push pptr down into the FROM_MERGE branch of the if/else statement,
where it's actually used, and call commit_list_append() for appending
elements instead of playing tricks with commit_list_insert().  Call
copy_commit_list() in the amend branch instead of open-coding it.  Don't
bother setting pptr in the final branch as it's not used thereafter.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/commit.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a2baa6e..8976c3d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1642,7 +1642,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	const char *index_file, *reflog_msg;
 	char *nl;
 	unsigned char sha1[20];
-	struct commit_list *parents = NULL, **pptr = &parents;
+	struct commit_list *parents = NULL;
 	struct stat statbuf;
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
@@ -1688,20 +1688,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (!reflog_msg)
 			reflog_msg = "commit (initial)";
 	} else if (amend) {
-		struct commit_list *c;
-
 		if (!reflog_msg)
 			reflog_msg = "commit (amend)";
-		for (c = current_head->parents; c; c = c->next)
-			pptr = &commit_list_insert(c->item, pptr)->next;
+		parents = copy_commit_list(current_head->parents);
 	} else if (whence == FROM_MERGE) {
 		struct strbuf m = STRBUF_INIT;
 		FILE *fp;
 		int allow_fast_forward = 1;
+		struct commit_list **pptr = &parents;
 
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
-		pptr = &commit_list_insert(current_head, pptr)->next;
+		pptr = commit_list_append(current_head, pptr);
 		fp = fopen(git_path_merge_head(), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
@@ -1712,7 +1710,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			parent = get_merge_parent(m.buf);
 			if (!parent)
 				die(_("Corrupt MERGE_HEAD file (%s)"), m.buf);
-			pptr = &commit_list_insert(parent, pptr)->next;
+			pptr = commit_list_append(parent, pptr);
 		}
 		fclose(fp);
 		strbuf_release(&m);
@@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reflog_msg = (whence == FROM_CHERRY_PICK)
 					? "commit (cherry-pick)"
 					: "commit";
-		pptr = &commit_list_insert(current_head, pptr)->next;
+		commit_list_insert(current_head, &parents);
 	}
 
 	/* Finally, get the commit message */
-- 
2.10.2

Re: [PATCH] commit: simplify building parents list

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Push pptr down into the FROM_MERGE branch of the if/else statement,
> where it's actually used, and call commit_list_append() for appending
> elements instead of playing tricks with commit_list_insert().  Call
> copy_commit_list() in the amend branch instead of open-coding it.  Don't
> bother setting pptr in the final branch as it's not used thereafter.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> ...
> @@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			reflog_msg = (whence == FROM_CHERRY_PICK)
>  					? "commit (cherry-pick)"
>  					: "commit";
> -		pptr = &commit_list_insert(current_head, pptr)->next;
> +		commit_list_insert(current_head, &parents);
>  	}

I needed to read the full preimage to determine why this hunk is
equivalent to the original.  Which is a good demonstration that what
motivated this patch is a valid issue to tackle---initializing the
pptr variable to point at &parents too early and have the long
if/elseif/... cascade work with it made the code unnecessarily
harder to understand and this update untangles that.

Thanks.