Re: [PATCH v5 04/12] replay: parse commits before dereferencing them
Patrick Steinhardt <ps@pks.im> writes:
Show 10 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.
>
> 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?
Show 18 quoted lines
> 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);
Junio C Hamano <gitster@pobox.com>