From: René Scharfe <l.s.r@web.de>
Date: Tue, 27 Oct 2015 21:29:50 GMT
Subject: Re: [PATCH] use pop_commit() for consuming the first entry of a struct commit_list
Message-ID: <562FECCE.8050207@web.de>
In-Reply-To: <xmqqfv0xfh6q.fsf@gitster.mtv.corp.google.com>

Am 26.10.2015 um 22:33 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> 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.

Yeah, it's tempting to doze off after a few similar cases.

> Let me try to go through it very carefully.
>
>> 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.

This is just to keep the line shorter than 80 characters, and it's not 
the first "cmit" in the tree..

>
>> 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.

With "git show -W" you don't need any magic number. ;)

Thanks for reviewing!

René