Subject: Re: [PATCH v2 3/7] log: do not free parents when walking reflog

From: Junio C Hamano <[email-masked]>
Date: Fri, 07 Jul 2017 10:10:54 -0700

Jeff King <[email-masked]> writes:

> When we're doing a reflog walk (instead of walking the
> actual parent pointers), we may see commits multiple times.
> For this reason, we hold on to the commit buffer for each
> commit rather than freeing it after we've showed the commit.
>
> We should do the same for the parent list. Right now this is
> just a minor optimization. But once we refactor how reflog
> walks are performed, keeping the parents will avoid
> confusing us the second time we see the commit.
>
> Signed-off-by: Jeff King <[email-masked]>
> ---
>  builtin/log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 8ca1de9894..9c8bb3b5c3 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -374,9 +374,9 @@ static int cmd_log_walk(struct rev_info *rev)
>  		if (!rev->reflog_info) {
>  			/* we allow cycles in reflog ancestry */
>  			free_commit_buffer(commit);
> +			free_commit_list(commit->parents);
> +			commit->parents = NULL;

After step 6/7, we no longer "allow cycles in reflog ancestry", as
there will be no reflog ancestry to speak of ;-), so it would be
nice to remove the comment above in that step.  But alternatively,
we can rephrase the comment here, to say something like "the same
commit can be shown multiple times while showing entries from the
reflog" instead.

>  		}
> -		free_commit_list(commit->parents);
> -		commit->parents = NULL;
>  		if (saved_nrl < rev->diffopt.needed_rename_limit)
>  			saved_nrl = rev->diffopt.needed_rename_limit;
>  		if (rev->diffopt.degraded_cc_to_c)