{"thread":{"id":"40633","subject":"[PATCH] use pop_commit() for consuming the first entry of a struct commit_list","startedAt":"2015-10-24T16:21:31Z","lastAt":"2015-10-27T21:29:50Z","messageCount":3,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"272171","messageId":"562BB00B.1000704@web.de","threadId":"40633","inReplyTo":null,"subject":"[PATCH] use pop_commit() for consuming the first entry of a struct commit_list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-24T16:21:31Z","receivedAt":"2015-10-24T16:21:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Instead of open-coding the function pop_commit() just call it.  This\nmakes the intent clearer and reduces code size.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/fmt-merge-msg.c |  9 +++------\n builtin/merge.c         | 12 +++++-------\n builtin/reflog.c        |  6 +-----\n builtin/rev-parse.c     |  7 ++-----\n builtin/show-branch.c   | 17 +++--------------\n commit.c                | 27 +++++++--------------------\n remote.c                |  6 ++----\n revision.c              | 27 +++++----------------------\n shallow.c               |  6 +-----\n upload-pack.c           |  6 ++----\n 10 files changed, 31 insertions(+), 92 deletions(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 4ba7f28..846004b 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n static void find_merge_parents(struct merge_parents *result,\n \t\t\t       struct strbuf *in, unsigned char *head)\n {\n-\tstruct commit_list *parents, *next;\n+\tstruct commit_list *parents;\n \tstruct commit *head_commit;\n \tint pos = 0, i, j;\n \n@@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents *result,\n \tparents = reduce_heads(parents);\n \n \twhile (parents) {\n+\t\tstruct commit *cmit = pop_commit(&parents);\n \t\tfor (i = 0; i < result->nr; i++)\n-\t\t\tif (!hashcmp(result->item[i].commit,\n-\t\t\t\t     parents->item->object.sha1))\n+\t\t\tif (!hashcmp(result->item[i].commit, cmit->object.sha1))\n \t\t\t\tresult->item[i].used = 1;\n-\t\tnext = parents->next;\n-\t\tfree(parents);\n-\t\tparents = next;\n \t}\n \n \tfor (i = j = 0; i < result->nr; i++) {\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a0a9328..31241b8 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n \t\t\t\t\t  int *head_subsumed,\n \t\t\t\t\t  struct commit_list *remoteheads)\n {\n-\tstruct commit_list *parents, *next, **remotes = &remoteheads;\n+\tstruct commit_list *parents, **remotes;\n \n \t/*\n \t * Is the current HEAD reachable from another commit being\n@@ -1033,16 +1033,14 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n \t/* Find what parents to record by checking independent ones. */\n \tparents = reduce_heads(remoteheads);\n \n-\tfor (remoteheads = NULL, remotes = &remoteheads;\n-\t     parents;\n-\t     parents = next) {\n-\t\tstruct commit *commit = parents->item;\n-\t\tnext = parents->next;\n+\tremoteheads = NULL;\n+\tremotes = &remoteheads;\n+\twhile (parents) {\n+\t\tstruct commit *commit = pop_commit(&parents);\n \t\tif (commit == head_commit)\n \t\t\t*head_subsumed = 0;\n \t\telse\n \t\t\tremotes = &commit_list_insert(commit, remotes)->next;\n-\t\tfree(parents);\n \t}\n \treturn remoteheads;\n }\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex f96ca2a..cf1145e 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -218,7 +218,6 @@ static int keep_entry(struct commit **it, unsigned char *sha1)\n  */\n static void mark_reachable(struct expire_reflog_policy_cb *cb)\n {\n-\tstruct commit *commit;\n \tstruct commit_list *pending;\n \tunsigned long expire_limit = cb->mark_limit;\n \tstruct commit_list *leftover = NULL;\n@@ -228,11 +227,8 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb)\n \n \tpending = cb->mark_list;\n \twhile (pending) {\n-\t\tstruct commit_list *entry = pending;\n \t\tstruct commit_list *parent;\n-\t\tpending = entry->next;\n-\t\tcommit = entry->item;\n-\t\tfree(entry);\n+\t\tstruct commit *commit = pop_commit(&pending);\n \t\tif (commit->object.flags & REACHABLE)\n \t\t\tcontinue;\n \t\tif (parse_commit(commit))\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 02d747d..e92a782 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -281,11 +281,8 @@ static int try_difference(const char *arg)\n \t\t\tb = lookup_commit_reference(end);\n \t\t\texclude = get_merge_bases(a, b);\n \t\t\twhile (exclude) {\n-\t\t\t\tstruct commit_list *n = exclude->next;\n-\t\t\t\tshow_rev(REVERSED,\n-\t\t\t\t\t exclude->item->object.sha1,NULL);\n-\t\t\t\tfree(exclude);\n-\t\t\t\texclude = n;\n+\t\t\t\tstruct commit *commit = pop_commit(&exclude);\n+\t\t\t\tshow_rev(REVERSED, commit->object.sha1, NULL);\n \t\t\t}\n \t\t}\n \t\t*dotdot = '.';\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 092b59b..ac5141d 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -53,17 +53,6 @@ static struct commit *interesting(struct commit_list *list)\n \treturn NULL;\n }\n \n-static struct commit *pop_one_commit(struct commit_list **list_p)\n-{\n-\tstruct commit *commit;\n-\tstruct commit_list *list;\n-\tlist = *list_p;\n-\tcommit = list->item;\n-\t*list_p = list->next;\n-\tfree(list);\n-\treturn commit;\n-}\n-\n struct commit_name {\n \tconst char *head_name; /* which head's ancestor? */\n \tint generation; /* how many parents away from head_name */\n@@ -213,7 +202,7 @@ static void join_revs(struct commit_list **list_p,\n \twhile (*list_p) {\n \t\tstruct commit_list *parents;\n \t\tint still_interesting = !!interesting(*list_p);\n-\t\tstruct commit *commit = pop_one_commit(list_p);\n+\t\tstruct commit *commit = pop_commit(list_p);\n \t\tint flags = commit->object.flags & all_mask;\n \n \t\tif (!still_interesting && extra <= 0)\n@@ -504,7 +493,7 @@ static int show_merge_base(struct commit_list *seen, int num_rev)\n \tint exit_status = 1;\n \n \twhile (seen) {\n-\t\tstruct commit *commit = pop_one_commit(&seen);\n+\t\tstruct commit *commit = pop_commit(&seen);\n \t\tint flags = commit->object.flags & all_mask;\n \t\tif (!(flags & UNINTERESTING) &&\n \t\t    ((flags & all_revs) == all_revs)) {\n@@ -929,7 +918,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \tall_revs = all_mask & ~((1u << REV_SHIFT) - 1);\n \n \twhile (seen) {\n-\t\tstruct commit *commit = pop_one_commit(&seen);\n+\t\tstruct commit *commit = pop_commit(&seen);\n \t\tint this_flag = commit->object.flags;\n \t\tint is_merge_point = ((this_flag & all_revs) == all_revs);\n \ndiff --git a/commit.c b/commit.c\nindex 494615d..d1810c9 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -455,11 +455,8 @@ struct commit_list *copy_commit_list(struct commit_list *list)\n \n void free_commit_list(struct commit_list *list)\n {\n-\twhile (list) {\n-\t\tstruct commit_list *temp = list;\n-\t\tlist = temp->next;\n-\t\tfree(temp);\n-\t}\n+\twhile (list)\n+\t\tpop_commit(&list);\n }\n \n struct commit_list * commit_list_insert_by_date(struct commit *item, struct commit_list **list)\n@@ -505,12 +502,8 @@ void commit_list_sort_by_date(struct commit_list **list)\n struct commit *pop_most_recent_commit(struct commit_list **list,\n \t\t\t\t      unsigned int mark)\n {\n-\tstruct commit *ret = (*list)->item;\n+\tstruct commit *ret = pop_commit(list);\n \tstruct commit_list *parents = ret->parents;\n-\tstruct commit_list *old = *list;\n-\n-\t*list = (*list)->next;\n-\tfree(old);\n \n \twhile (parents) {\n \t\tstruct commit *commit = parents->item;\n@@ -861,11 +854,9 @@ static struct commit_list *merge_bases_many(struct commit *one, int n, struct co\n \tlist = paint_down_to_common(one, n, twos);\n \n \twhile (list) {\n-\t\tstruct commit_list *next = list->next;\n-\t\tif (!(list->item->object.flags & STALE))\n-\t\t\tcommit_list_insert_by_date(list->item, &result);\n-\t\tfree(list);\n-\t\tlist = next;\n+\t\tstruct commit *commit = pop_commit(&list);\n+\t\tif (!(commit->object.flags & STALE))\n+\t\t\tcommit_list_insert_by_date(commit, &result);\n \t}\n \treturn result;\n }\n@@ -1546,13 +1537,9 @@ int commit_tree_extended(const char *msg, size_t msg_len,\n \t * if everything else stays the same.\n \t */\n \twhile (parents) {\n-\t\tstruct commit_list *next = parents->next;\n-\t\tstruct commit *parent = parents->item;\n-\n+\t\tstruct commit *parent = pop_commit(&parents);\n \t\tstrbuf_addf(&buffer, \"parent %s\\n\",\n \t\t\t    sha1_to_hex(parent->object.sha1));\n-\t\tfree(parents);\n-\t\tparents = next;\n \t}\n \n \t/* Person/date information */\ndiff --git a/remote.c b/remote.c\nindex 1101f82..977e34e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1938,10 +1938,8 @@ int resolve_remote_symref(struct ref *ref, struct ref *list)\n static void unmark_and_free(struct commit_list *list, unsigned int mark)\n {\n \twhile (list) {\n-\t\tstruct commit_list *temp = list;\n-\t\ttemp->item->object.flags &= ~mark;\n-\t\tlist = temp->next;\n-\t\tfree(temp);\n+\t\tstruct commit *commit = pop_commit(&list);\n+\t\tcommit->object.flags &= ~mark;\n \t}\n }\n \ndiff --git a/revision.c b/revision.c\nindex 2236463..0fbb684 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -153,10 +153,7 @@ void mark_parents_uninteresting(struct commit *commit)\n \t\tcommit_list_insert(l->item, &parents);\n \n \twhile (parents) {\n-\t\tstruct commit *commit = parents->item;\n-\t\tl = parents;\n-\t\tparents = parents->next;\n-\t\tfree(l);\n+\t\tstruct commit *commit = pop_commit(&parents);\n \n \t\twhile (commit) {\n \t\t\t/*\n@@ -1102,14 +1099,10 @@ static int limit_list(struct rev_info *revs)\n \t}\n \n \twhile (list) {\n-\t\tstruct commit_list *entry = list;\n-\t\tstruct commit *commit = list->item;\n+\t\tstruct commit *commit = pop_commit(&list);\n \t\tstruct object *obj = &commit->object;\n \t\tshow_early_output_fn_t show;\n \n-\t\tlist = list->next;\n-\t\tfree(entry);\n-\n \t\tif (commit == interesting_cache)\n \t\t\tinteresting_cache = NULL;\n \n@@ -2733,10 +2726,7 @@ static void simplify_merges(struct rev_info *revs)\n \t\tyet_to_do = NULL;\n \t\ttail = &yet_to_do;\n \t\twhile (list) {\n-\t\t\tcommit = list->item;\n-\t\t\tnext = list->next;\n-\t\t\tfree(list);\n-\t\t\tlist = next;\n+\t\t\tcommit = pop_commit(&list);\n \t\t\ttail = simplify_one(revs, commit, tail);\n \t\t}\n \t}\n@@ -2748,10 +2738,7 @@ static void simplify_merges(struct rev_info *revs)\n \twhile (list) {\n \t\tstruct merge_simplify_state *st;\n \n-\t\tcommit = list->item;\n-\t\tnext = list->next;\n-\t\tfree(list);\n-\t\tlist = next;\n+\t\tcommit = pop_commit(&list);\n \t\tst = locate_simplify_state(revs, commit);\n \t\tif (st->simplified == commit)\n \t\t\ttail = &commit_list_insert(commit, tail)->next;\n@@ -3125,11 +3112,7 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\treturn NULL;\n \n \tdo {\n-\t\tstruct commit_list *entry = revs->commits;\n-\t\tstruct commit *commit = entry->item;\n-\n-\t\trevs->commits = entry->next;\n-\t\tfree(entry);\n+\t\tstruct commit *commit = pop_commit(&revs->commits);\n \n \t\tif (revs->reflog_info) {\n \t\t\tsave_parents(revs, commit);\ndiff --git a/shallow.c b/shallow.c\nindex d49a3d6..4dcb454 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -401,13 +401,9 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,\n \tcommit_list_insert(c, &head);\n \twhile (head) {\n \t\tstruct commit_list *p;\n-\t\tstruct commit *c = head->item;\n+\t\tstruct commit *c = pop_commit(&head);\n \t\tuint32_t **refs = ref_bitmap_at(&info->ref_bitmap, c);\n \n-\t\tp = head;\n-\t\thead = head->next;\n-\t\tfree(p);\n-\n \t\t/* XXX check \"UNINTERESTING\" from pack bitmaps if available */\n \t\tif (c->object.flags & (SEEN | UNINTERESTING))\n \t\t\tcontinue;\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 89e832b..d0bc3ca 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -316,10 +316,8 @@ static int reachable(struct commit *want)\n \n \tcommit_list_insert_by_date(want, &work);\n \twhile (work) {\n-\t\tstruct commit_list *list = work->next;\n-\t\tstruct commit *commit = work->item;\n-\t\tfree(work);\n-\t\twork = list;\n+\t\tstruct commit_list *list;\n+\t\tstruct commit *commit = pop_commit(&work);\n \n \t\tif (commit->object.flags & THEY_HAVE) {\n \t\t\twant->object.flags |= COMMON_KNOWN;\n-- \n2.6.2\n"},{"id":"272270","messageId":"xmqqfv0xfh6q.fsf@gitster.mtv.corp.google.com","threadId":"40633","inReplyTo":"562BB00B.1000704@web.de","subject":"Re: [PATCH] use pop_commit() for consuming the first entry of a struct commit_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-26T21:33:49Z","receivedAt":"2015-10-26T21:33:49Z","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> Instead of open-coding the function pop_commit() just call it.  This\n> makes the intent clearer and reduces code size.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  builtin/fmt-merge-msg.c |  9 +++------\n>  builtin/merge.c         | 12 +++++-------\n>  builtin/reflog.c        |  6 +-----\n>  builtin/rev-parse.c     |  7 ++-----\n>  builtin/show-branch.c   | 17 +++--------------\n>  commit.c                | 27 +++++++--------------------\n>  remote.c                |  6 ++----\n>  revision.c              | 27 +++++----------------------\n>  shallow.c               |  6 +-----\n>  upload-pack.c           |  6 ++----\n>  10 files changed, 31 insertions(+), 92 deletions(-)\n\nWhile I appreciate this kind of simplification very much, I also\nhate to review this kind of simplification at the same time, as it\nis very easy to make and miss simple mistakes.\n\nLet me try to go through it very carefully.\n\n> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\n> index 4ba7f28..846004b 100644\n> --- a/builtin/fmt-merge-msg.c\n> +++ b/builtin/fmt-merge-msg.c\n> @@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n>  static void find_merge_parents(struct merge_parents *result,\n>  \t\t\t       struct strbuf *in, unsigned char *head)\n>  {\n> -\tstruct commit_list *parents, *next;\n> +\tstruct commit_list *parents;\n>  \tstruct commit *head_commit;\n>  \tint pos = 0, i, j;\n>  \n> @@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents *result,\n>  \tparents = reduce_heads(parents);\n>  \n>  \twhile (parents) {\n> +\t\tstruct commit *cmit = pop_commit(&parents);\n>  \t\tfor (i = 0; i < result->nr; i++)\n> -\t\t\tif (!hashcmp(result->item[i].commit,\n> -\t\t\t\t     parents->item->object.sha1))\n> +\t\t\tif (!hashcmp(result->item[i].commit, cmit->object.sha1))\n>  \t\t\t\tresult->item[i].used = 1;\n> -\t\tnext = parents->next;\n> -\t\tfree(parents);\n> -\t\tparents = next;\n\nOK, I would have called it \"commit\" not \"cmit\", but this is\ntrivially equivalent to the original.\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index a0a9328..31241b8 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n>  \t\t\t\t\t  int *head_subsumed,\n>  \t\t\t\t\t  struct commit_list *remoteheads)\n>  {\n> -\tstruct commit_list *parents, *next, **remotes = &remoteheads;\n> +\tstruct commit_list *parents, **remotes;\n\nInteresting.  I viewed the result of applying this patch with \"git\nshow -U32\" to make sure that this initialization of remotes was\nunnecessary.\n\n> @@ -1033,16 +1033,14 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n>  \t/* Find what parents to record by checking independent ones. */\n>  \tparents = reduce_heads(remoteheads);\n>  \n> -\tfor (remoteheads = NULL, remotes = &remoteheads;\n> -\t     parents;\n> -\t     parents = next) {\n> -\t\tstruct commit *commit = parents->item;\n> -\t\tnext = parents->next;\n> +\tremoteheads = NULL;\n> +\tremotes = &remoteheads;\n> +\twhile (parents) {\n> +\t\tstruct commit *commit = pop_commit(&parents);\n>  \t\tif (commit == head_commit)\n>  \t\t\t*head_subsumed = 0;\n>  \t\telse\n>  \t\t\tremotes = &commit_list_insert(commit, remotes)->next;\n> -\t\tfree(parents);\n>  \t}\n>  \treturn remoteheads;\n>  }\n\nTrivially equivalent.  Good.  I'll stop saying this if there is\nnothing noteworthy from now on.\n\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 092b59b..ac5141d 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -53,17 +53,6 @@ static struct commit *interesting(struct commit_list *list)\n>  \treturn NULL;\n>  }\n>  \n> -static struct commit *pop_one_commit(struct commit_list **list_p)\n> -{\n> -\tstruct commit *commit;\n> -\tstruct commit_list *list;\n> -\tlist = *list_p;\n> -\tcommit = list->item;\n> -\t*list_p = list->next;\n> -\tfree(list);\n> -\treturn commit;\n> -}\n> -\n\nInteresting.  We had our own implementation that is less lenient\nthan the global one.  And this one is newer by two months!  Good\nriddance.\n\n> @@ -2733,10 +2726,7 @@ static void simplify_merges(struct rev_info *revs)\n>  \t\tyet_to_do = NULL;\n>  \t\ttail = &yet_to_do;\n>  \t\twhile (list) {\n> -\t\t\tcommit = list->item;\n> -\t\t\tnext = list->next;\n> -\t\t\tfree(list);\n> -\t\t\tlist = next;\n> +\t\t\tcommit = pop_commit(&list);\n>  \t\t\ttail = simplify_one(revs, commit, tail);\n>  \t\t}\n>  \t}\n\nAny conversion in this set that does not eliminate the intermediate\nvariable \"next\" (or named similarly but differently in some\ncontexts) is suspect, but this is correct.  The variable is used in\nan earlier loop to walk a list in an non-destructive way (strictly\nspeaking, I do not think that loop needs 'next' variable, though).\n\n> diff --git a/shallow.c b/shallow.c\n> index d49a3d6..4dcb454 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -401,13 +401,9 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,\n>  \tcommit_list_insert(c, &head);\n>  \twhile (head) {\n>  \t\tstruct commit_list *p;\n> -\t\tstruct commit *c = head->item;\n> +\t\tstruct commit *c = pop_commit(&head);\n>  \t\tuint32_t **refs = ref_bitmap_at(&info->ref_bitmap, c);\n>  \n> -\t\tp = head;\n> -\t\thead = head->next;\n> -\t\tfree(p);\n> -\n>  \t\t/* XXX check \"UNINTERESTING\" from pack bitmaps if available */\n>  \t\tif (c->object.flags & (SEEN | UNINTERESTING))\n>  \t\t\tcontinue;\n\nAgain, 'p' is used in another loop in the same scope, and the\nconversion is correct.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 89e832b..d0bc3ca 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -316,10 +316,8 @@ static int reachable(struct commit *want)\n>  \n>  \tcommit_list_insert_by_date(want, &work);\n>  \twhile (work) {\n> -\t\tstruct commit_list *list = work->next;\n> -\t\tstruct commit *commit = work->item;\n> -\t\tfree(work);\n> -\t\twork = list;\n> +\t\tstruct commit_list *list;\n> +\t\tstruct commit *commit = pop_commit(&work);\n>  \n>  \t\tif (commit->object.flags & THEY_HAVE) {\n>  \t\t\twant->object.flags |= COMMON_KNOWN;\n\nLikewise, \"list\" is used in the same scope for different purposes,\nand the conversion is correct.\n\nThanks, will apply.\n"},{"id":"272346","messageId":"562FECCE.8050207@web.de","threadId":"40633","inReplyTo":"xmqqfv0xfh6q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] use pop_commit() for consuming the first entry of a struct commit_list","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-10-27T21:29:50Z","receivedAt":"2015-10-27T21:29:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.10.2015 um 22:33 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Instead of open-coding the function pop_commit() just call it.  This\n>> makes the intent clearer and reduces code size.\n>>\n>> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n>> ---\n>>   builtin/fmt-merge-msg.c |  9 +++------\n>>   builtin/merge.c         | 12 +++++-------\n>>   builtin/reflog.c        |  6 +-----\n>>   builtin/rev-parse.c     |  7 ++-----\n>>   builtin/show-branch.c   | 17 +++--------------\n>>   commit.c                | 27 +++++++--------------------\n>>   remote.c                |  6 ++----\n>>   revision.c              | 27 +++++----------------------\n>>   shallow.c               |  6 +-----\n>>   upload-pack.c           |  6 ++----\n>>   10 files changed, 31 insertions(+), 92 deletions(-)\n>\n> While I appreciate this kind of simplification very much, I also\n> hate to review this kind of simplification at the same time, as it\n> is very easy to make and miss simple mistakes.\n\nYeah, it's tempting to doze off after a few similar cases.\n\n> Let me try to go through it very carefully.\n>\n>> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\n>> index 4ba7f28..846004b 100644\n>> --- a/builtin/fmt-merge-msg.c\n>> +++ b/builtin/fmt-merge-msg.c\n>> @@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n>>   static void find_merge_parents(struct merge_parents *result,\n>>   \t\t\t       struct strbuf *in, unsigned char *head)\n>>   {\n>> -\tstruct commit_list *parents, *next;\n>> +\tstruct commit_list *parents;\n>>   \tstruct commit *head_commit;\n>>   \tint pos = 0, i, j;\n>>\n>> @@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents *result,\n>>   \tparents = reduce_heads(parents);\n>>\n>>   \twhile (parents) {\n>> +\t\tstruct commit *cmit = pop_commit(&parents);\n>>   \t\tfor (i = 0; i < result->nr; i++)\n>> -\t\t\tif (!hashcmp(result->item[i].commit,\n>> -\t\t\t\t     parents->item->object.sha1))\n>> +\t\t\tif (!hashcmp(result->item[i].commit, cmit->object.sha1))\n>>   \t\t\t\tresult->item[i].used = 1;\n>> -\t\tnext = parents->next;\n>> -\t\tfree(parents);\n>> -\t\tparents = next;\n>\n> OK, I would have called it \"commit\" not \"cmit\", but this is\n> trivially equivalent to the original.\n\nThis is just to keep the line shorter than 80 characters, and it's not \nthe first \"cmit\" in the tree..\n\n>\n>> diff --git a/builtin/merge.c b/builtin/merge.c\n>> index a0a9328..31241b8 100644\n>> --- a/builtin/merge.c\n>> +++ b/builtin/merge.c\n>> @@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n>>   \t\t\t\t\t  int *head_subsumed,\n>>   \t\t\t\t\t  struct commit_list *remoteheads)\n>>   {\n>> -\tstruct commit_list *parents, *next, **remotes = &remoteheads;\n>> +\tstruct commit_list *parents, **remotes;\n>\n> Interesting.  I viewed the result of applying this patch with \"git\n> show -U32\" to make sure that this initialization of remotes was\n> unnecessary.\n\nWith \"git show -W\" you don't need any magic number. ;)\n\nThanks for reviewing!\n\nRené\n"}]}