Re: [PATCH] commit: simplify building parents list

[Junio C Hamano]

René Scharfe  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 
> ---
> ...
> @@ -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 = _list_insert(current_head, pptr)->next;
> + commit_list_insert(current_head, );
>   }

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  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.

[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 
---
 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 = 
+   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 = _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 = 
 
if (!reflog_msg)
reflog_msg = "commit (merge)";
-   pptr = _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 = _list_insert(parent, pptr)->next;
+   pptr = commit_list_append(parent, pptr);
}
fclose(fp);
strbuf_release();
@@ -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 = _list_insert(current_head, pptr)->next;
+   commit_list_insert(current_head, );
}
 
/* Finally, get the commit message */
-- 
2.10.2