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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 27, 2025, 09:57 UTC
Message-ID: <aP9CGN39LSaFU-Ru@pks.im>
In-Reply-To: <xmqqo6q0t1kd.fsf@gitster.g>

On Tue, Oct 21, 2025 at 01:57:54PM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> 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?

There's only a single caller in "builtin/replay.c", and that caller parses commits deep inside the callstack via `prepare_revision_walk()`. So we cannot easily get rid of any calls.

>  * 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?

Not quite sure I understand this question. In any case, I was hitting segfaults in tests when I didn't have this call. But your questions made me double-check this now, and I cannot see any of these failures anymore. And we do use the same infra to pick commits as "builtin/replay.c" does, so things should work alright.

Let me drop this commit for now. Things work without it, and in theory they should. And now that I'm revamping the infra to not use the merge machinery in the first place we don't even hit this code path anymore.

Thanks!

Patrick