Re: [PATCH] use pop_commit() for consuming the first entry of a struct commit_list
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 26, 2015, 21:33 UTC
- Message-ID
- <xmqqfv0xfh6q.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <562BB00B.1000704@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 16 quoted lines
> 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 <l.s.r@web.de> > --- > 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.
Show 26 quoted lines
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index 4ba7f28..846004b 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -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;OK, I would have called it "commit" not "cmit", but this is trivially equivalent to the original.
Show 10 quoted lines
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a0a9328..31241b8 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -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.
Show 21 quoted lines
> @@ -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;
> }Trivially equivalent. Good. I'll stop saying this if there is nothing noteworthy from now on.
Show 19 quoted lines
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 092b59b..ac5141d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -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;
> -}
> -Interesting. We had our own implementation that is less lenient than the global one. And this one is newer by two months! Good riddance.
Show 12 quoted lines
> @@ -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).
Show 19 quoted lines
> diff --git a/shallow.c b/shallow.c
> index d49a3d6..4dcb454 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -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;Again, 'p' is used in another loop in the same scope, and the conversion is correct.
Show 17 quoted lines
> diff --git a/upload-pack.c b/upload-pack.c
> index 89e832b..d0bc3ca 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -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;Likewise, "list" is used in the same scope for different purposes, and the conversion is correct.
Thanks, will apply.