Re: [PATCH] use pop_commit() for consuming the first entry of a struct commit_list
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 27, 2015, 21:29 UTC
- 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:
Show 22 quoted lines
> 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.
Show 31 quoted lines
> 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..
Show 15 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.With "git show -W" you don't need any magic number. ;)
Thanks for reviewing!
René