threads / patch / 64300

v2[Outreachy] commit.c: clarify comment describing commit re-parse behavior

Subject: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior

## tl;dr

4 messages between Oct 11, 2025 and Oct 14, 2025. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Okhuomon Ajayi· Oct 11, 2025, 09:36 UTC · lore

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 <okhuomonajayi54@gmail.com>
---
 commit.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Show changes to commit.c +3 −4

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· Oct 13, 2025, 09:43 UTC · re: Okhuomon Ajayi · lore

Re: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior

On Sat, Oct 11, 2025 at 11:36 AM Okhuomon Ajayi <okhuomonajayi54@gmail.com> wrote:

Show 7 quoted lines

>
> 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..."

Show 5 quoted lines

> to ensure a clean state before re-parsing, which improves readability
> and avoids misunderstanding during future maintenance.
>
> Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
> ---

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· Oct 14, 2025, 00:35 UTC · re: Okhuomon Ajayi · lore

Re: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior

On Sat, Oct 11, 2025 at 10:36:11AM +0100, Okhuomon Ajayi wrote:

Show 8 quoted lines

> 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· Oct 14, 2025, 00:41 UTC · re: Jeff King · lore

Re: [PATCH v2] [Outreachy] commit.c: clarify comment describing commit re-parse behavior

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 <peff@peff.net> wrote:

Show 16 quoted lines

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