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

Gatla Vishweshwar Reddy <[email protected]> writes:

> show-branch uses commit->object.flags to store per-branch
> ...
> Signed-off-by: Gatla Vishweshwar Reddy <[email protected]>
> ---
>
>> 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.