Re: [PATCH v2 0/3] libgit-rs: add get_bool() method to ConfigSet

[Junio C Hamano <gitster@pobox.com>]

"ions via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Purpose
>
> This pull request introduces a get_bool() method to the ConfigSet module in
> the libgit-rs library. The goal is to enhance the functionality of ConfigSet
> by providing a way to fetch and handle boolean configuration values more
> easily and consistently.
>
> Implementation Details
>
> • Added a get_bool() method to the ConfigSet module.
>
> • The method retrieves configuration values as boolean values, ensuring
> proper parsing and error handling.
>
> • This addition simplifies the process of working with boolean
> configurations for developers using the ConfigSet module.
>
> Testing
>
> • Added unit tests to verify the correctness of the get_bool() method.
>
> • Tested edge cases to ensure robustness.

Somebody may suggest a better way to organize your cover letter for
second and subsequent iterations, but it is helpful to returning
readers to list what changed since the previous iteration.

> ionnss (3):
>   po: fix escaped underscores in README.md
>   libgit-rs: add get_bool() method to ConfigSet
>   libgit-rs: address review feedback for get_bool()

I am not sure if you really wanted to make this a thre-patch series.
The README.md patch looks pretty much independent and irrelevant to
the Rust topic to me.  I am not a GGG user, but there must be a way
to tell it to send out a two-patch series from this branch.

> base-commit: bb69721404348ea2db0a081c41ab6ebfe75bdec8

Perhaps you'd need to rebase the top two commit on top of this
bb697214 (The twelfth batch, 2025-09-23).  Currently your two
commits that belong to this libgit-rs series are not built on top of
that commit in the mainline.  They are built on top of d7810781fc,
as can be seen below.

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1977%2Fionnss%2Fadd-rust-configset-get-bool-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1977/ionnss/add-rust-configset-get-bool-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1977
>
> Range-diff vs v1:
>
>  1:  d7810781fc = 1:  d7810781fc po: fix escaped underscores in README.md
>  2:  a5904a2ac0 = 2:  a5904a2ac0 libgit-rs: add get_bool() method to ConfigSet
>  -:  ---------- > 3:  43784e3ff9 libgit-rs: address review feedback for get_bool()

Actually, I do not think you want a two-patch series, either.

Review comments (sorry, I do not recall the details) must have
pointed out what was wrong in the initial iteration, but this
iteration is carrying the same badness without correcting anyting in
a5904a2ac0 and resending it with "oops that was wrong in the
previous step, and here is a band-aid on top" commit that is the new
43784e3ff9.

Until there is a concensus that your topic is ready and gets merged
to 'next', you have the luxury to pretend as if you are a perfect
developer who never made such mistakes that can easily be pointed
out on the mailing list by others.  In short, anything in 43784e3ff9
that removes lines that were added in a5904a2ac0 and replaces them
with something else are unwanted "I went there, it was a bad idea,
here I am correcting the earlier mistake" changes.

And the way to pretend that you are a perfect developer is to
rewrite (i.e. "commit --amend") your "add get_bool()" with the new
strategy to wrap the C version used by real Git, instead of
reimplementing it manually in Rust.  This is a chance you have to
pretend that you weren't even tempted to reimplement the thing in
Rust and went straight to the right solution.  So grab that chance.
Once your topic gets merged to 'next', you no longer have that
opportunity to wholesale replace your patches to pretend that you
never made mistakes.

This is because nobody reading "git log -p" later is interested in
your (or anybody's) mistakes that have already been corrected.  It
might record your cherished memory to celebrate your growth as a
developer, but that is not something the history of a public project
is used for.

After your topic is merged to 'next', we'd go incremental.  This is
because being in 'next' means that enough reviewers agreed that a
particular iteration is good enough.  If later it was found that
they are not good enough and need corrections, such a mistake that
nobody managed to spot during the review becomes worth recording and
its correction worth describing as a separate patch on top.

But we are not there yet with this series.

Thanks.