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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> 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.
>
> Make the function easier to use by calling `repo_parse_commit()`.

Two-and-half obvious questions.

 * With this change, can we lose the parse-commit call(s) from
   existing callers, or do the need to look at the in-core commit
   object themselves before calling this function so they need to
   have their parse-commit call(s) anyway?

 * Can new callers you plan to add decide without having an already
   parsed "pickme" commit object if they want to call this function,
   iow, can they decide to call or not to call this function without
   looking at the members of the commit structure?

 * Are existing callers prepared to see NULL returned from this
   function to signal an error?

> 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 13d75d80543..c3628d2488b 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);