Re: [PATCH v2] advice: suggest using subcommand "git config set"

From: Rubén Justo <rjusto@gmail.com>
Date: Dec 9, 2024, 20:35 UTC
Message-ID: <be4ee78e-12d4-44c2-9f82-4f0db7706fea@gmail.com>
In-Reply-To: <D6791Z2QPSUW.1LP269FO886XF@ferdinandy.com>

On Mon, Dec 09, 2024 at 03:46:04PM +0100, Bence Ferdinandy wrote:

Show 5 quoted lines

> I started to split the commit, but realized that I only updated "git config
> advice\." to "git config set advice." in the tests. If I split the around five
> instances of actually using "git config advice" in the code, then it starts to
> make a lot less sense for why it is only for "advice" and not for all the other
> uses of "git config" in the tests.

If I understand the intention of this series correctly, the main goal is to update the help messages we give to the user on how to disable the advice messages. I think you have addressed that.

Updating the tests to use the new UI "git config set advice" sounds in this series, because it's related to the advice machinery.

Updating the test suite to use the new "git config" UI seems out of scope, I think.

Show 8 quoted lines

> So I'm now inclined to think that I either
> leave the patch as is, or simple just remove the parts that are not updating
> expected test outcomes and leave updating usage of "git config" in tests for
> a later as it would likely be a larger effort to clean up everything to use
> explicit set/get. This cleanup would also only make sense if there are plans to
> deprecate the old implicit setting syntax at some point.
> 
> So should I remove the changes to usage in tests or just leave the patch as is?

I don't have a strong opinion on this. Since my message, Junio has marked this series to be merged to "next". I can be perfectly happy with the patch as is.

On the other hand, perhaps I could send my patches about `advise_if_enabled()`, later, rebuilt on this series once the dust has settled.