Re: [PATCH v4 04/12] replay: parse commits before dereferencing them

From: Karthik Nayak <karthik.188@gmail.com>
Date: Oct 14, 2025, 08:57 UTC
Message-ID: <CAOLa=ZTU7JvqiDqDK0gHbR1KshZ8A_rZgguNZykcHp2i--GQAw@mail.gmail.com>
In-Reply-To: <20251001-b4-pks-history-builtin-v4-4-8e61ddb86317@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 9 quoted lines

> When looking up a commit it may not be parsed yet. Callers that wish to
> access the fields of `struct commit` have to call `repo_parse_commit()`
> first so that it is guaranteed to be populated.
>
> We didn't yet care about doing so, because code paths that lead to
> `pick_regular_commit()` in "builtin/replay.c" already implicitly parsed
> the commits. But now that the function is exposed to outside callers
> it's quite easy to get this wrong.
>

So I was wondering, wouldn't this duplicate the call made to `pick_regular_commit()` and end up parsing the commit twice. But seems like down the stack in `repo_parse_commit_internal()`, we check for `item->object.parsed` and only parse if it hasn't been already parsed. So this change is welcome.

Show 24 quoted lines

> Make the function easier to use by calling `repo_parse_commit()`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  replay.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/replay.c b/replay.c
> index 13d75d8054..c3628d2488 100644
> --- a/replay.c
> +++ b/replay.c
> @@ -90,6 +90,9 @@ struct commit *replay_pick_regular_commit(struct repository *repo,
>  	struct commit *base, *replayed_base;
>  	struct tree *pickme_tree, *base_tree;
>
> +	if (repo_parse_commit(repo, pickme))
> +		return NULL;
> +
>  	base = pickme->parents->item;
>  	replayed_base = mapped_commit(replayed_commits, base, onto);
>
>
> --
> 2.51.0.700.g236ee7b076.dirty