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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2017, 17:10 UTC
Message-ID: <xmqqwp7kb2ap.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170707090734.x2ki7lluawf66g4a@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 25 quoted lines

> 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 <peff@peff.net>
> ---
>  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.

Show 6 quoted lines

>  		}
> -		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)