use pop_commit() for consuming the first entry of a struct commit_list
PATCH 1/1 use pop_commit() for consuming the first entry of a struct commit_list
Instead of open-coding the function pop_commit() just call it. This makes the intent clearer and reduces code size.
--- builtin/fmt-merge-msg.c | 9 +++------ builtin/merge.c | 12 +++++------- builtin/reflog.c | 6 +----- builtin/rev-parse.c | 7 ++----- builtin/show-branch.c | 17 +++-------------- commit.c | 27 +++++++-------------------- remote.c | 6 ++---- revision.c | 27 +++++---------------------- shallow.c | 6 +----- upload-pack.c | 6 ++---- 10 files changed, 31 insertions(+), 92 deletions(-)
builtin/fmt-merge-msg.c +3 -6
@@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)static void find_merge_parents (struct merge_parents *result,struct strbuf *in,unsigned char *head) { -struct commit_list *parents, *next; +struct commit_list *parents;struct commit *head_commit;int pos =0 , i, j;
@@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents *result, parents =reduce_heads (parents);while (parents) { +struct commit *cmit =pop_commit (&parents);for (i =0 ; i < result->nr; i++) -if (!hashcmp (result->item[i].commit, - parents->item->object.sha1)) +if (!hashcmp (result->item[i].commit, cmit->object.sha1)) result->item[i].used =1 ; - next = parents->next; -free (parents); - parents = next; }for (i = j =0 ; i < result->nr; i++) {
builtin/merge.c +5 -7
@@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,int *head_subsumed, struct commit_list *remoteheads) { - struct commit_list *parents, *next, **remotes = &remoteheads; + struct commit_list *parents, **remotes;/* * Is the current HEAD reachable from another commit being
@@ -1033,16 +1033,14 @@ static struct commit_list *reduce_parents(struct commit *head_commit,/* Find what parents to record by checking independent ones. */ parents =reduce_heads (remoteheads); -for (remoteheads =NULL , remotes = &remoteheads; - parents; - parents = next) { -struct commit *commit = parents->item; - next = parents->next; + remoteheads =NULL ; + remotes = &remoteheads; +while (parents) { +struct commit *commit =pop_commit (&parents);if (commit == head_commit) *head_subsumed =0 ;else remotes = &commit_list_insert (commit, remotes)->next; -free (parents); }return remoteheads; }
builtin/reflog.c +1 -5
@@ -218,7 +218,6 @@ static int keep_entry(struct commit **it, unsigned char *sha1)*/ static void mark_reachable (struct expire_reflog_policy_cb *cb) { -struct commit *commit;struct commit_list *pending;unsigned long expire_limit = cb->mark_limit;struct commit_list *leftover =NULL ;
@@ -228,11 +227,8 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb) pending = cb->mark_list;while (pending) { -struct commit_list *entry = pending;struct commit_list *parent; - pending = entry->next; - commit = entry->item; -free (entry); +struct commit *commit =pop_commit (&pending);if (commit->object.flags &REACHABLE )continue ;if (parse_commit (commit))
builtin/rev-parse.c +2 -5
@@ -281,11 +281,8 @@ static int try_difference(const char *arg) b =lookup_commit_reference (end); exclude =get_merge_bases (a, b);while (exclude) { -struct commit_list *n = exclude->next; -show_rev (REVERSED , - exclude->item->object.sha1,NULL ); -free (exclude); - exclude = n; +struct commit *commit =pop_commit (&exclude); +show_rev (REVERSED , commit->object.sha1,NULL ); } } *dotdot ='.' ;
builtin/show-branch.c +3 -14
@@ -53,17 +53,6 @@ static struct commit *interesting(struct commit_list *list)return NULL ; } -static struct commit *pop_one_commit (struct commit_list **list_p) -{ -struct commit *commit; -struct commit_list *list; - list = *list_p; - commit = list->item; - *list_p = list->next; -free (list); -return commit; -} -struct commit_name {const char *head_name;/* which head's ancestor? */ int generation;/* how many parents away from head_name */
@@ -213,7 +202,7 @@ static void join_revs(struct commit_list **list_p,while (*list_p) {struct commit_list *parents;int still_interesting = !!interesting (*list_p); -struct commit *commit =pop_one_commit (list_p); +struct commit *commit =pop_commit (list_p);int flags = commit->object.flags & all_mask;if (!still_interesting && extra <=0 )
@@ -504,7 +493,7 @@ static int show_merge_base(struct commit_list *seen, int num_rev)int exit_status =1 ;while (seen) { -struct commit *commit =pop_one_commit (&seen); +struct commit *commit =pop_commit (&seen);int flags = commit->object.flags & all_mask;if (!(flags &UNINTERESTING ) && ((flags & all_revs) == all_revs)) {
@@ -929,7 +918,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) all_revs = all_mask & ~((1u <<REV_SHIFT ) -1 );while (seen) { -struct commit *commit =pop_one_commit (&seen); +struct commit *commit =pop_commit (&seen);int this_flag = commit->object.flags;int is_merge_point = ((this_flag & all_revs) == all_revs);
commit.c +7 -20
@@ -455,11 +455,8 @@ struct commit_list *copy_commit_list(struct commit_list *list)void free_commit_list (struct commit_list *list) { -while (list) { -struct commit_list *temp = list; - list = temp->next; -free (temp); - } +while (list) +pop_commit (&list); }struct commit_list *commit_list_insert_by_date (struct commit *item,struct commit_list **list)
@@ -505,12 +502,8 @@ void commit_list_sort_by_date(struct commit_list **list)struct commit *pop_most_recent_commit (struct commit_list **list,unsigned int mark) { -struct commit *ret = (*list)->item; +struct commit *ret =pop_commit (list);struct commit_list *parents = ret->parents; -struct commit_list *old = *list; - - *list = (*list)->next; -free (old);while (parents) {struct commit *commit = parents->item;
@@ -861,11 +854,9 @@ static struct commit_list *merge_bases_many(struct commit *one, int n, struct co list =paint_down_to_common (one, n, twos);while (list) { -struct commit_list *next = list->next; -if (!(list->item->object.flags &STALE )) -commit_list_insert_by_date (list->item, &result); -free (list); - list = next; +struct commit *commit =pop_commit (&list); +if (!(commit->object.flags &STALE )) +commit_list_insert_by_date (commit, &result); }return result; }
@@ -1546,13 +1537,9 @@ int commit_tree_extended(const char *msg, size_t msg_len,* if everything else stays the same. */ while (parents) { -struct commit_list *next = parents->next; -struct commit *parent = parents->item; - +struct commit *parent =pop_commit (&parents);strbuf_addf (&buffer,"parent %s\n" ,sha1_to_hex (parent->object.sha1)); -free (parents); - parents = next; }/* Person/date information */
remote.c +2 -4
@@ -1938,10 +1938,8 @@ int resolve_remote_symref(struct ref *ref, struct ref *list)static void unmark_and_free (struct commit_list *list,unsigned int mark) {while (list) { -struct commit_list *temp = list; - temp->item->object.flags &= ~mark; - list = temp->next; -free (temp); +struct commit *commit =pop_commit (&list); + commit->object.flags &= ~mark; } }
revision.c +5 -22
@@ -153,10 +153,7 @@ void mark_parents_uninteresting(struct commit *commit)commit_list_insert (l->item, &parents);while (parents) { -struct commit *commit = parents->item; - l = parents; - parents = parents->next; -free (l); +struct commit *commit =pop_commit (&parents);while (commit) {/*
@@ -1102,14 +1099,10 @@ static int limit_list(struct rev_info *revs) }while (list) { -struct commit_list *entry = list; -struct commit *commit = list->item; +struct commit *commit =pop_commit (&list);struct object *obj = &commit->object;show_early_output_fn_t show; - list = list->next; -free (entry); -if (commit == interesting_cache) interesting_cache =NULL ;
@@ -2733,10 +2726,7 @@ static void simplify_merges(struct rev_info *revs) yet_to_do =NULL ; tail = &yet_to_do;while (list) { - commit = list->item; - next = list->next; -free (list); - list = next; + commit =pop_commit (&list); tail =simplify_one (revs, commit, tail); } }
@@ -2748,10 +2738,7 @@ static void simplify_merges(struct rev_info *revs)while (list) {struct merge_simplify_state *st; - commit = list->item; - next = list->next; -free (list); - list = next; + commit =pop_commit (&list); st =locate_simplify_state (revs, commit);if (st->simplified == commit) tail = &commit_list_insert (commit, tail)->next;
@@ -3125,11 +3112,7 @@ static struct commit *get_revision_1(struct rev_info *revs)return NULL ; do { - struct commit_list *entry = revs->commits; - struct commit *commit = entry->item; - - revs->commits = entry->next; - free(entry); + struct commit *commit = pop_commit(&revs->commits); if (revs->reflog_info) { save_parents(revs, commit);
shallow.c +1 -5
@@ -401,13 +401,9 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,commit_list_insert (c, &head);while (head) {struct commit_list *p; -struct commit *c = head->item; +struct commit *c =pop_commit (&head);uint32_t **refs =ref_bitmap_at (&info->ref_bitmap, c); - p = head; - head = head->next; -free (p); -/* XXX check "UNINTERESTING" from pack bitmaps if available */ if (c->object.flags & (SEEN |UNINTERESTING ))continue ;
upload-pack.c +2 -4
@@ -316,10 +316,8 @@ static int reachable(struct commit *want)commit_list_insert_by_date (want, &work);while (work) { -struct commit_list *list = work->next; -struct commit *commit = work->item; -free (work); - work = list; +struct commit_list *list; +struct commit *commit =pop_commit (&work);if (commit->object.flags &THEY_HAVE ) { want->object.flags |=COMMON_KNOWN ;
Signature · 1 line
2.6.2
Quoted 4 lines · René Scharfe
Instead of open-coding the function
pop_commit()just call it. This makes the intent clearer and reduces code size.Signed-off-by Rene Scharfe <[email protected]>
Quoted 12 lines
--- builtin/fmt-merge-msg.c | 9 +++------ builtin/merge.c | 12 +++++------- builtin/reflog.c | 6 +----- builtin/rev-parse.c | 7 ++----- builtin/show-branch.c | 17 +++-------------- commit.c | 27 +++++++-------------------- remote.c | 6 ++---- revision.c | 27 +++++---------------------- shallow.c | 6 +----- upload-pack.c | 6 ++---- 10 files changed, 31 insertions(+), 92 deletions(-)
While I appreciate this kind of simplification very much, I also hate to review this kind of simplification at the same time, as it is very easy to make and miss simple mistakes.
Let me try to go through it very carefully.
Quoted 16 lines
builtin/fmt-merge-msg.c +1
-1@@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)static void find_merge_parents (struct merge_parents *result,struct strbuf *in,unsigned char *head) { -struct commit_list *parents, *next; +struct commit_list *parents;struct commit *head_commit;int pos =0 , i, j;@@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents *result, parents =reduce_heads (parents);
builtin/fmt-merge-msg.c static void find_merge_parents(struct merge_parents *result, · line 585
Quoted 10 lines
builtin/fmt-merge-msg.c +2
-5while (parents) { +struct commit *cmit =pop_commit (&parents);for (i =0 ; i < result->nr; i++) -if (!hashcmp (result->item[i].commit, - parents->item->object.sha1)) +if (!hashcmp (result->item[i].commit, cmit->object.sha1)) result->item[i].used =1 ; - next = parents->next; -free (parents); - parents = next;
OK, I would have called it "commit" not "cmit", but this is trivially equivalent to the original.
builtin/merge.c static struct commit_list *reduce_parents(struct commit *head_commit, · line 1022
Quoted 10 lines
builtin/merge.c +1
-1@@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,int *head_subsumed, struct commit_list *remoteheads) { - struct commit_list *parents, *next, **remotes = &remoteheads; + struct commit_list *parents, **remotes;
Interesting. I viewed the result of applying this patch with "git show -U32" to make sure that this initialization of remotes was unnecessary.
Quoted 11 lines
builtin/merge.c +2
-5@@ -1033,16 +1033,14 @@ static struct commit_list *reduce_parents(struct commit *head_commit,/* Find what parents to record by checking independent ones. */ parents =reduce_heads (remoteheads); -for (remoteheads =NULL , remotes = &remoteheads; - parents; - parents = next) { -struct commit *commit = parents->item; - next = parents->next; + remoteheads =NULL ; + remotes = &remoteheads;
builtin/merge.c static struct commit_list *reduce_parents(struct commit *head_commit, · line 1046
Quoted 10 lines
builtin/merge.c +2
-1+while (parents) { +struct commit *commit =pop_commit (&parents);if (commit == head_commit) *head_subsumed =0 ;else remotes = &commit_list_insert (commit, remotes)->next; -free (parents); }return remoteheads; }
Trivially equivalent. Good. I'll stop saying this if there is nothing noteworthy from now on.
Quoted 9 lines
builtin/show-branch.c +0
-1@@ -53,17 +53,6 @@ static struct commit *interesting(struct commit_list *list)return NULL ; } -static struct commit *pop_one_commit (struct commit_list **list_p)
builtin/show-branch.c static struct commit *interesting(struct commit_list *list) · line 66
Quoted 10 lines
builtin/show-branch.c +0
-10-{ -struct commit *commit; -struct commit_list *list; - list = *list_p; - commit = list->item; - *list_p = list->next; -free (list); -return commit; -} -
Interesting. We had our own implementation that is less lenient than the global one. And this one is newer by two months! Good riddance.
builtin/show-branch.c static void simplify_merges(struct rev_info *revs) · line 2732
Quoted 12 lines
builtin/show-branch.c +1
-4@@ -2733,10 +2726,7 @@ static void simplify_merges(struct rev_info *revs) yet_to_do =NULL ; tail = &yet_to_do;while (list) { - commit = list->item; - next = list->next; -free (list); - list = next; + commit =pop_commit (&list); tail =simplify_one (revs, commit, tail); } }
Any conversion in this set that does not eliminate the intermediate variable "next" (or named similarly but differently in some contexts) is suspect, but this is correct. The variable is used in an earlier loop to walk a list in an non-destructive way (strictly speaking, I do not think that loop needs 'next' variable, though).
Quoted 9 lines
shallow.c +0
-1@@ -401,13 +401,9 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,commit_list_insert (c, &head);while (head) {struct commit_list *p; -struct commit *c = head->item;
shallow.c static void paint_down(struct paint_info *info, const unsigned char *sha1, · line 409
Quoted 10 lines
shallow.c +1
-4+struct commit *c =pop_commit (&head);uint32_t **refs =ref_bitmap_at (&info->ref_bitmap, c); - p = head; - head = head->next; -free (p); -/* XXX check "UNINTERESTING" from pack bitmaps if available */ if (c->object.flags & (SEEN |UNINTERESTING ))continue ;
Again, 'p' is used in another loop in the same scope, and the conversion is correct.
Quoted 7 lines
upload-pack.c +0
-0@@ -316,10 +316,8 @@ static int reachable(struct commit *want)commit_list_insert_by_date (want, &work);
upload-pack.c static int reachable(struct commit *want) · line 323
Quoted 10 lines
upload-pack.c +2
-4while (work) { -struct commit_list *list = work->next; -struct commit *commit = work->item; -free (work); - work = list; +struct commit_list *list; +struct commit *commit =pop_commit (&work);if (commit->object.flags &THEY_HAVE ) { want->object.flags |=COMMON_KNOWN ;
Likewise, "list" is used in the same scope for different purposes, and the conversion is correct.
Thanks, will apply.
Am 26.10.2015 um 22:33 schrieb Junio C Hamano:
Quoted 19 lines
Quoted 4 lines · René Scharfe
Instead of open-coding the function
pop_commit()just call it. This makes the intent clearer and reduces code size.Signed-off-by Rene Scharfe <[email protected]>Quoted 12 lines
--- builtin/fmt-merge-msg.c | 9 +++------ builtin/merge.c | 12 +++++------- builtin/reflog.c | 6 +----- builtin/rev-parse.c | 7 ++----- builtin/show-branch.c | 17 +++-------------- commit.c | 27 +++++++-------------------- remote.c | 6 ++---- revision.c | 27 +++++---------------------- shallow.c | 6 +----- upload-pack.c | 6 ++---- 10 files changed, 31 insertions(+), 92 deletions(-)
Quoted 3 lines
While I appreciate this kind of simplification very much, I also hate to review this kind of simplification at the same time, as it is very easy to make and miss simple mistakes.
Yeah, it's tempting to doze off after a few similar cases.
Quoted 18 lines
Let me try to go through it very carefully.
Quoted 16 lines
builtin/fmt-merge-msg.c +1
-1@@ -537,7 +537,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)static void find_merge_parents (struct merge_parents *result,struct strbuf *in,unsigned char *head) { -struct commit_list *parents, *next; +struct commit_list *parents;struct commit *head_commit;int pos =0 , i, j;@@ -576,13 +576,10 @@ static void find_merge_parents(struct merge_parents *result, parents =reduce_heads (parents);
Quoted 13 lines
builtin/fmt-merge-msg.c static void find_merge_parents(struct merge_parents *result, · line 585
Quoted 10 lines
builtin/fmt-merge-msg.c +2
-5while (parents) { +struct commit *cmit =pop_commit (&parents);for (i =0 ; i < result->nr; i++) -if (!hashcmp (result->item[i].commit, - parents->item->object.sha1)) +if (!hashcmp (result->item[i].commit, cmit->object.sha1)) result->item[i].used =1 ; - next = parents->next; -free (parents); - parents = next;OK, I would have called it "commit" not "cmit", but this is trivially equivalent to the original.
This is just to keep the line shorter than 80 characters, and it's not the first "cmit" in the tree..
Quoted 15 lines
builtin/merge.c static struct commit_list *reduce_parents(struct commit *head_commit, · line 1022
Quoted 11 lines
builtin/merge.c +1
-1@@ -1019,7 +1019,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,int *head_subsumed, struct commit_list *remoteheads) { - struct commit_list *parents, *next, **remotes = &remoteheads; + struct commit_list *parents, **remotes;Interesting. I viewed the result of applying this patch with "git show -U32" to make sure that this initialization of remotes was unnecessary.
With "git show -W" you don't need any magic number. ;)
Thanks for reviewing!
René