# [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior 4 messages from 2025-10-11 to 2025-10-14. Participants: Okhuomon Ajayi, Christian Couder, Jeff King. Thread: https://gitlist.dev/t/64300 ## Okhuomon Ajayi, 2025-10-11 09:36 Subject: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior Message-ID: <20251011093611.62937-1-okhuomonajayi54@gmail.com> URL: https://gitlist.dev/e/20251011093611.62937-1-okhuomonajayi54%40gmail.com ``` The existing comment in `parse_commit_buffer()` mentioned a "leftover from an earlier failed parse", which could be confusing to new readers. It implied an error state rather than the intended cleanup before re-parsing a commit. Clarifying the comment makes it explicit that we reset the parent list to ensure a clean state before re-parsing, which improves readability and avoids misunderstanding during future maintenance. Signed-off-by: Okhuomon Ajayi --- commit.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/commit.c b/commit.c index 16d91b2bfc..af20ca7c3d 100644 --- a/commit.c +++ b/commit.c @@ -475,10 +475,9 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b if (item->object.parsed) return 0; /* - * Presumably this is leftover from an earlier failed parse; - * clear it out in preparation for us re-parsing (we'll hit the - * same error, but that's good, since it lets our caller know - * the result cannot be trusted. + * Reset the parent list before re-parsing to ensure a clear + * commit state. This avoids carrying over data from a previous + * incomplete or invalid parse. */ free_commit_list(item->parents); item->parents = NULL; -- 2.43.0 ``` ## Christian Couder, 2025-10-13 09:43 Subject: Re: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior Message-ID: URL: https://gitlist.dev/e/CAP8UFD3nn%3Dn3XLRKjrHpMOM4uf3wCFGMjdy13wOp%3D_vZHTeYWw%40mail.gmail.com In-Reply-To: <20251011093611.62937-1-okhuomonajayi54@gmail.com> ``` On Sat, Oct 11, 2025 at 11:36 AM Okhuomon Ajayi wrote: > > The existing comment in `parse_commit_buffer()` mentioned a "leftover > from an earlier failed parse", which could be confusing to new readers. > It implied an error state rather than the intended cleanup before > re-parsing a commit. > > Clarifying the comment makes it explicit that we reset the parent list We use an imperative mood to describe what a patch does. (See the "imperative-mood" section of Documentation/SubmittingPatches.) So maybe: "Clarify the comment to make it explicit..." or "Let's clarify the comment to make it explicit..." > to ensure a clean state before re-parsing, which improves readability > and avoids misunderstanding during future maintenance. > > Signed-off-by: Okhuomon Ajayi > --- When looking at your email on the mailing list archive: https://lore.kernel.org/git/20251011093611.62937-1-okhuomonajayi54@gmail.com/T/#u it looks like your message is the only one in its thread. So it's not easy to understand why it's a "v2", and what the corresponding v1 is. To link a message to a previous one, there is the "In-Reply-To:" email header. It can be added using the `--in-reply-to='...'` command line option if you use `git send-email` to send emails. (Not sure how to do it with GigGitGadget.) At the very least, you could add a regular link to this part of your email (after line containing only three dash characters above) maybe like this: https://lore.kernel.org/git/20251010233303.783212-1-okhuomonajayi54@gmail.com/ By the way, in this part of your email patches, if there is no cover letter where you already do it, it's a good idea to explain the context of the patch, and when it's not the first version, to list the changes compared to the previous version and often to provide a range-diff. (See Documentation/SubmittingPatches about the cover letter.) > commit.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) Thanks. ``` ## Jeff King, 2025-10-14 00:35 Subject: Re: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior Message-ID: <20251014003508.GD1507@coredump.intra.peff.net> URL: https://gitlist.dev/e/20251014003508.GD1507%40coredump.intra.peff.net In-Reply-To: <20251011093611.62937-1-okhuomonajayi54@gmail.com> ``` On Sat, Oct 11, 2025 at 10:36:11AM +0100, Okhuomon Ajayi wrote: > The existing comment in `parse_commit_buffer()` mentioned a "leftover > from an earlier failed parse", which could be confusing to new readers. > It implied an error state rather than the intended cleanup before > re-parsing a commit. > > Clarifying the comment makes it explicit that we reset the parent list > to ensure a clean state before re-parsing, which improves readability > and avoids misunderstanding during future maintenance. As the original author of this comment, I think what you've written retains the intent but is easier to understand. So looks good to me. -Peff ``` ## Okhuomon Ajayi, 2025-10-14 00:41 Subject: Re: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior Message-ID: URL: https://gitlist.dev/e/CAFpMFfCKimM9zWODGgEnA962C%2Bi4nCBL41JcRcX48yqwY%2BjtcQ%40mail.gmail.com In-Reply-To: <20251014003508.GD1507@coredump.intra.peff.net> ``` Thanks a lot for the feedback, Christian and Jeff! I’ll reword the commit message to use the imperative mood and add a short changelog with a link to the previous versions below the ‘---’ line. On Tue, Oct 14, 2025 at 1:35 AM Jeff King wrote: > > On Sat, Oct 11, 2025 at 10:36:11AM +0100, Okhuomon Ajayi wrote: > > > The existing comment in `parse_commit_buffer()` mentioned a "leftover > > from an earlier failed parse", which could be confusing to new readers. > > It implied an error state rather than the intended cleanup before > > re-parsing a commit. > > > > Clarifying the comment makes it explicit that we reset the parent list > > to ensure a clean state before re-parsing, which improves readability > > and avoids misunderstanding during future maintenance. > > As the original author of this comment, I think what you've written > retains the intent but is easier to understand. So looks good to me. > > -Peff ```