# [PATCH] commit: simplify building parents list 2 messages from 2016-10-29 to 2016-10-30. Participants: René Scharfe, Junio C Hamano. Thread: https://gitlist.dev/t/44400 ## René Scharfe, 2016-10-29 12:55 Subject: [PATCH] commit: simplify building parents list Message-ID: <30afdbeb-7a45-70b2-495e-35fd3b62419a@web.de> URL: https://gitlist.dev/e/30afdbeb-7a45-70b2-495e-35fd3b62419a%40web.de ``` 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 = &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 ``` ## Junio C Hamano, 2016-10-30 23:03 Subject: Re: [PATCH] commit: simplify building parents list Message-ID: URL: https://gitlist.dev/e/xmqqd1ihv3rp.fsf%40gitster.mtv.corp.google.com In-Reply-To: <30afdbeb-7a45-70b2-495e-35fd3b62419a@web.de> ``` 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 = &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. ```