{"thread":{"id":"44400","subject":"[PATCH] commit: simplify building parents list","startedAt":"2016-10-29T12:55:52Z","lastAt":"2016-10-30T23:03:31Z","messageCount":2,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"305164","messageId":"30afdbeb-7a45-70b2-495e-35fd3b62419a@web.de","threadId":"44400","inReplyTo":null,"subject":"[PATCH] commit: simplify building parents list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-29T12:55:36Z","receivedAt":"2016-10-29T12:55:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Push pptr down into the FROM_MERGE branch of the if/else statement,\nwhere it's actually used, and call commit_list_append() for appending\nelements instead of playing tricks with commit_list_insert().  Call\ncopy_commit_list() in the amend branch instead of open-coding it.  Don't\nbother setting pptr in the final branch as it's not used thereafter.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/commit.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a2baa6e..8976c3d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1642,7 +1642,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tconst char *index_file, *reflog_msg;\n \tchar *nl;\n \tunsigned char sha1[20];\n-\tstruct commit_list *parents = NULL, **pptr = &parents;\n+\tstruct commit_list *parents = NULL;\n \tstruct stat statbuf;\n \tstruct commit *current_head = NULL;\n \tstruct commit_extra_header *extra = NULL;\n@@ -1688,20 +1688,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (initial)\";\n \t} else if (amend) {\n-\t\tstruct commit_list *c;\n-\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (amend)\";\n-\t\tfor (c = current_head->parents; c; c = c->next)\n-\t\t\tpptr = &commit_list_insert(c->item, pptr)->next;\n+\t\tparents = copy_commit_list(current_head->parents);\n \t} else if (whence == FROM_MERGE) {\n \t\tstruct strbuf m = STRBUF_INIT;\n \t\tFILE *fp;\n \t\tint allow_fast_forward = 1;\n+\t\tstruct commit_list **pptr = &parents;\n \n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n-\t\tpptr = &commit_list_insert(current_head, pptr)->next;\n+\t\tpptr = commit_list_append(current_head, pptr);\n \t\tfp = fopen(git_path_merge_head(), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n@@ -1712,7 +1710,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tparent = get_merge_parent(m.buf);\n \t\t\tif (!parent)\n \t\t\t\tdie(_(\"Corrupt MERGE_HEAD file (%s)\"), m.buf);\n-\t\t\tpptr = &commit_list_insert(parent, pptr)->next;\n+\t\t\tpptr = commit_list_append(parent, pptr);\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n@@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n \t\t\t\t\t? \"commit (cherry-pick)\"\n \t\t\t\t\t: \"commit\";\n-\t\tpptr = &commit_list_insert(current_head, pptr)->next;\n+\t\tcommit_list_insert(current_head, &parents);\n \t}\n \n \t/* Finally, get the commit message */\n-- \n2.10.2\n\n"},{"id":"305193","messageId":"xmqqd1ihv3rp.fsf@gitster.mtv.corp.google.com","threadId":"44400","inReplyTo":"30afdbeb-7a45-70b2-495e-35fd3b62419a@web.de","subject":"Re: [PATCH] commit: simplify building parents list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-30T23:03:06Z","receivedAt":"2016-10-30T23:03:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Push pptr down into the FROM_MERGE branch of the if/else statement,\n> where it's actually used, and call commit_list_append() for appending\n> elements instead of playing tricks with commit_list_insert().  Call\n> copy_commit_list() in the amend branch instead of open-coding it.  Don't\n> bother setting pptr in the final branch as it's not used thereafter.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> ...\n> @@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\n>  \t\t\t\t\t? \"commit (cherry-pick)\"\n>  \t\t\t\t\t: \"commit\";\n> -\t\tpptr = &commit_list_insert(current_head, pptr)->next;\n> +\t\tcommit_list_insert(current_head, &parents);\n>  \t}\n\nI needed to read the full preimage to determine why this hunk is\nequivalent to the original.  Which is a good demonstration that what\nmotivated this patch is a valid issue to tackle---initializing the\npptr variable to point at &parents too early and have the long\nif/elseif/... cascade work with it made the code unnecessarily\nharder to understand and this update untangles that.\n\nThanks.\n\n\n"}]}