Re: [PATCH v10] show-branch: convert per-branch flags to commit-slab

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2026, 20:44 UTC
Message-ID: <xmqqo6fxe8rf.fsf@gitster.g>
In-Reply-To: <20260721203025.85044-1-gatlavishweshwarreddy26@gmail.com>

Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:

Show 24 quoted lines

> show-branch uses commit->object.flags to store per-branch
> ...
> Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
> ---
>
>> Hmph. I hate to say this, but I am finding it difficult to trust
>> your "carefully" at this point.
>>
>>     $ make
>>     $ ./git show-branch master next
>>     Floating point exception (core dumped).
>
> You are right to not trust it. I missed this completely. I ran the
> full test suite but did not run the binary manually before sending.
> That was the wrong approach. I have now run every mode manually
> before sending this version.
> ...
> All tests pass. No crashes in any mode.
>
> ---
> Changes in v10:
> - Restore init_commit_name_slab(&name_slab) before repo_config()
>   that was accidentally dropped in v7. Without it, name_slab.slab_size
>   is 0 causing division by zero on first commit lookup.

I will not read the contents of v10, but I think it is worth setting some expectations first. I am usually pretty patient, but even my patience has its limits.

First and foremost, this development community is built on humans collaborating with other humans. An author posts a patch, a reviewer responds with suggestions or critiques, and the author replies to that e-mail. In their own words, the author might:

 - build on the suggestion, rephrasing it and proposing further
   improvements;

 - disagree and offer a counter-proposal;

 - concede the patch's shortcomings and outline how they plan to fix
   them; or

 - defend their original design to give the reviewer a chance to
   reconsider.

Doing this in your own words helps reviewers see how close we are to an agreement. This kind of discussion often needs a few rounds of back-and-forth. It should also welcome folks watching from the sidelines, which means letting the globe spin at least once so developers in other timezones can chime in before we declare a rough consensus.

Firing off a new iteration before there is a rough consensus on what it should look like is a total waste of everyone's time.

Finally, the space below the three-dash line is absolutely not the place to conduct a discussion. Those debates belong in separate, threaded e-mail replies. Use the space to remind readers that this work is based on a consensus achieved in an earlier thread [*].

Also, to be clear, I didn't bring up the core dump because I was upset about a lack of testing [**]. We are all error-prone humans, and mistakes (like dropping an unrelated line) happen to the best of us. Maybe a cat distracts you, and while your head is turned, you accidentally hit dd (or C-k for the Emacs crowd) and delete a line without realizing it.

No, the real issue was that this deletion should have leaped out at anyone reading the patch, immediately prompting some questions:

    We are removing this initialization.  Why?  Have we changed the
    API to make BSS initialization sufficient?  Does the updated
    code no longer use this structure?  Do we initialize it
    somewhere else now?

And until those questions are answered, no one can honestly claim to have 'reviewed the patch carefully.'

It is perfectly fine to have some fun letting AI assistants write code for you. However, please make sure you are prepared to explain every single change in the patch when asked. It is already a bit of a philosophical stretch to call a patch 'yours' when an AI did the heavy lifting, but it definitely is not yours if you cannot explain it in your own words. If you are not yet familiar with the codebase, it is OK if you do not have all the answers right away. Just hold off on sending the patch until you do.

A suggestion I can give users of AI assistants is to have your AI assistant actually help you. And by that, I do not mean tossing it a lazy, one-line prompt like 'please explain every line in this patch.' Instead, read through its output yourself, line by line and hunk by hunk, and ask yourself if you can explain why each change exists. If you can't, ask the AI. If you don't understand its answer, grill it further in your own words, using the actual questions that pop into your head.

Here is a fun little exercise you might enjoy. If you can resurrect and continue the chat session with the AI agent that spawned the v9 patch, ask it why it decided to delete that init_commit_name_slab() call, and what it thought the ramifications of doing so would be.

I actually spotted a few more issues in the previous round, but I left them out of my review. Why? Because I expected you would just feed my feedback straight to your AI assistant, tell it to 'compose a response and update the patch,' and call it a day. And as Patrick pointed out earlier, none of us want to waste our brain cycles playing telephone with a human middleman who is just copy-pasting between an AI generator and the mailing list.

So, there.

[Footnotes]

 * This is a total tangent, but as I am ranting here, this is
   exactly why I hate seeing 'X requested this change' below the
   three-dash line.  Sure, the critique or suggestion might have
   originated with a reviewer, but by the time the author writes an
   updated iteration, it has become something both of them agree on.
   At that point, it is no longer a mere 'request' because the
   author is now just as much on board and backing the change as the
   reviewer.

** If anything, this episode exposed a massive gap in our test
   coverage, since the test suite completely missed a breakage in
   such a basic use of the command.  We may need to extend our test
   coverage before making further changes.