{"thread":{"id":"62598","subject":"[PATCH] advice: suggest using subcommand \"git config set\"","startedAt":"2024-12-04T13:15:42Z","lastAt":"2024-12-11T18:00:43Z","messageCount":16,"participants":["Bence Ferdinandy","Justin Tobler","Patrick Steinhardt","Junio C Hamano","Rubén Justo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508615","messageId":"20241204130928.1059851-1-bence@ferdinandy.com","threadId":"62598","inReplyTo":null,"subject":"[PATCH] advice: suggest using subcommand \"git config set\"","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-12-04T13:08:47Z","receivedAt":"2024-12-04T13:15:42Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"The advice message currently suggests using \"git config advice...\" to\ndisable advice messages, but since 00bbdde141f we have the \"set\"\nsubcommand for config. Change the disable advice message to use the\nsubcommand instead. Change all uses of \"git config advice\" in the tests\nto use the subcommand.\n\nSigned-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n---\n\nNotes:\n    For the tests I just indiscriminately ran:\n    sed -i \"s/git config advice\\./git config set advice./\" t[0-9]*.sh\n\n advice.c                        | 2 +-\n t/t0018-advice.sh               | 2 +-\n t/t3200-branch.sh               | 2 +-\n t/t3404-rebase-interactive.sh   | 6 +++---\n t/t3501-revert-cherry-pick.sh   | 2 +-\n t/t3507-cherry-pick-conflict.sh | 6 +++---\n t/t3510-cherry-pick-sequence.sh | 2 +-\n t/t3511-cherry-pick-x.sh        | 2 +-\n t/t3602-rm-sparse-checkout.sh   | 2 +-\n t/t3700-add.sh                  | 6 +++---\n t/t3705-add-sparse-checkout.sh  | 2 +-\n t/t7002-mv-sparse-checkout.sh   | 4 ++--\n t/t7004-tag.sh                  | 2 +-\n t/t7201-co.sh                   | 4 ++--\n t/t7400-submodule-basic.sh      | 2 +-\n t/t7508-status.sh               | 2 +-\n 16 files changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 6b879d805c..f7a5130c2c 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -93,7 +93,7 @@ static struct {\n \n static const char turn_off_instructions[] =\n N_(\"\\n\"\n-   \"Disable this message with \\\"git config advice.%s false\\\"\");\n+   \"Disable this message with \\\"git config set advice.%s false\\\"\");\n \n static void vadvise(const char *advice, int display_instructions,\n \t\t    const char *key, va_list params)\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 9a3db02fde..f68e08d0b1 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -10,7 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n test_expect_success 'advice should be printed when config variable is unset' '\n \tcat >expect <<-\\EOF &&\n \thint: This is a piece of advice\n-\thint: Disable this message with \"git config advice.nestedTag false\"\n+\thint: Disable this message with \"git config set advice.nestedTag false\"\n \tEOF\n \ttest-tool advise \"This is a piece of advice\" 2>actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 2295db3dcb..a3a21c54cf 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1696,7 +1696,7 @@ test_expect_success 'errors if given a bad branch name' '\n \tcat <<-\\EOF >expect &&\n \tfatal: '\\''foo..bar'\\'' is not a valid branch name\n \thint: See `man git check-ref-format`\n-\thint: Disable this message with \"git config advice.refSyntax false\"\n+\thint: Disable this message with \"git config set advice.refSyntax false\"\n \tEOF\n \ttest_must_fail git branch foo..bar >actual 2>&1 &&\n \ttest_cmp expect actual\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b11f04eb33..ecfc02062c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2258,20 +2258,20 @@ test_expect_success 'non-merge commands reject merge commits' '\n \terror: ${SQ}pick${SQ} does not accept merge commits\n \thint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to\n \thint: replay the merge, use ${SQ}merge -C${SQ} on the commit.\n-\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\thint: Disable this message with \"git config set advice.rebaseTodoError false\"\n \terror: invalid line 1: pick $oid\n \terror: ${SQ}reword${SQ} does not accept merge commits\n \thint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to\n \thint: replay the merge and reword the commit message, use\n \thint: ${SQ}merge -c${SQ} on the commit\n-\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\thint: Disable this message with \"git config set advice.rebaseTodoError false\"\n \terror: invalid line 2: reword $oid\n \terror: ${SQ}edit${SQ} does not accept merge commits\n \thint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to\n \thint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then\n \thint: ${SQ}break${SQ} to give the control back to you so that you can\n \thint: do ${SQ}git commit --amend && git rebase --continue${SQ}.\n-\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\thint: Disable this message with \"git config set advice.rebaseTodoError false\"\n \terror: invalid line 3: edit $oid\n \terror: cannot squash merge commit into another commit\n \terror: invalid line 4: fixup $oid\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 17a9937962..78b03d769d 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -177,7 +177,7 @@ test_expect_success 'advice from failed revert' '\n \thint: You can instead skip this commit with \"git revert --skip\".\n \thint: To abort and get back to the state before \"git revert\",\n \thint: run \"git revert --abort\".\n-\thint: Disable this message with \"git config advice.mergeConflict false\"\n+\thint: Disable this message with \"git config set advice.mergeConflict false\"\n \tEOF\n \ttest_commit --append --no-tag \"double-add dream\" dream dream &&\n \ttest_must_fail git revert HEAD^ 2>actual &&\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex f3947b400a..44596cb1e8 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -34,7 +34,7 @@ test_expect_success setup '\n \tgit commit --allow-empty --allow-empty-message &&\n \tgit tag empty &&\n \tgit checkout main &&\n-\tgit config advice.detachedhead false\n+\tgit config set advice.detachedhead false\n \n '\n \n@@ -60,7 +60,7 @@ test_expect_success 'advice from failed cherry-pick' '\n \thint: You can instead skip this commit with \"git cherry-pick --skip\".\n \thint: To abort and get back to the state before \"git cherry-pick\",\n \thint: run \"git cherry-pick --abort\".\n-\thint: Disable this message with \"git config advice.mergeConflict false\"\n+\thint: Disable this message with \"git config set advice.mergeConflict false\"\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n@@ -75,7 +75,7 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \terror: could not apply \\$picked... picked\n \thint: after resolving the conflicts, mark the corrected paths\n \thint: with 'git add <paths>' or 'git rm <paths>'\n-\thint: Disable this message with \\\"git config advice.mergeConflict false\\\"\n+\thint: Disable this message with \\\"git config set advice.mergeConflict false\\\"\n \tEOF\n \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 7eb52b12ed..66ff9db270 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -25,7 +25,7 @@ pristine_detach () {\n }\n \n test_expect_success setup '\n-\tgit config advice.detachedhead false &&\n+\tgit config set advice.detachedhead false &&\n \techo unrelated >unrelated &&\n \tgit add unrelated &&\n \ttest_commit initial foo a &&\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 84a587daf3..98ef13f0a3 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -51,7 +51,7 @@ trailing empty lines\n \"\n \n test_expect_success setup '\n-\tgit config advice.detachedhead false &&\n+\tgit config set advice.detachedhead false &&\n \techo unrelated >unrelated &&\n \tgit add unrelated &&\n \ttest_commit initial foo a &&\ndiff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh\nindex 08580fd3dc..02c7acd617 100755\n--- a/t/t3602-rm-sparse-checkout.sh\n+++ b/t/t3602-rm-sparse-checkout.sh\n@@ -20,7 +20,7 @@ test_expect_success 'setup' \"\n \thint: If you intend to update such entries, try one of the following:\n \thint: * Use the --sparse option.\n \thint: * Disable or modify the sparsity rules.\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \n \techo b | cat sparse_error_header - >sparse_entry_b_error &&\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 4c543a1a7e..df580a5806 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -31,7 +31,7 @@ test_expect_success 'Test with no pathspecs' '\n \tcat >expect <<-EOF &&\n \tNothing specified, nothing added.\n \thint: Maybe you wanted to say ${SQ}git add .${SQ}?\n-\thint: Disable this message with \"git config advice.addEmptyPathspec false\"\n+\thint: Disable this message with \"git config set advice.addEmptyPathspec false\"\n \tEOF\n \tgit add 2>actual &&\n \ttest_cmp expect actual\n@@ -375,7 +375,7 @@ test_expect_success '\"git add\" a embedded repository' '\n \t\thint: \tgit rm --cached inner1\n \t\thint:\n \t\thint: See \"git help submodule\" for more information.\n-\t\thint: Disable this message with \"git config advice.addEmbeddedRepo false\"\n+\t\thint: Disable this message with \"git config set advice.addEmbeddedRepo false\"\n \t\twarning: adding embedded git repository: inner2\n \t\tEOF\n \t\ttest_cmp expect actual\n@@ -413,7 +413,7 @@ cat >expect.err <<\\EOF\n The following paths are ignored by one of your .gitignore files:\n ignored-file\n hint: Use -f if you really want to add them.\n-hint: Disable this message with \"git config advice.addIgnoredFile false\"\n+hint: Disable this message with \"git config set advice.addIgnoredFile false\"\n EOF\n cat >expect.out <<\\EOF\n add 'track-this'\ndiff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh\nindex 2bade9e804..53a4782267 100755\n--- a/t/t3705-add-sparse-checkout.sh\n+++ b/t/t3705-add-sparse-checkout.sh\n@@ -54,7 +54,7 @@ test_expect_success 'setup' \"\n \thint: If you intend to update such entries, try one of the following:\n \thint: * Use the --sparse option.\n \thint: * Disable or modify the sparsity rules.\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \n \techo sparse_entry | cat sparse_error_header - >sparse_entry_error &&\ndiff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh\nindex 26582ae4e5..4d3f221224 100755\n--- a/t/t7002-mv-sparse-checkout.sh\n+++ b/t/t7002-mv-sparse-checkout.sh\n@@ -32,7 +32,7 @@ test_expect_success 'setup' \"\n \thint: If you intend to update such entries, try one of the following:\n \thint: * Use the --sparse option.\n \thint: * Disable or modify the sparsity rules.\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \n \tcat >dirty_error_header <<-EOF &&\n@@ -45,7 +45,7 @@ test_expect_success 'setup' \"\n \thint: To correct the sparsity of these paths, do the following:\n \thint: * Use \\\"git add --sparse <paths>\\\" to update the index\n \thint: * Use \\\"git sparse-checkout reapply\\\" to apply the sparsity rules\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \"\n \ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b1316e62f4..7cd5e16dc8 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1850,7 +1850,7 @@ test_expect_success 'recursive tagging should give advice' '\n \thint: already a tag. If you meant to tag the object that it points to, use:\n \thint:\n \thint: \tgit tag -f nested annotated-v4.0^{}\n-\thint: Disable this message with \"git config advice.nestedTag false\"\n+\thint: Disable this message with \"git config set advice.nestedTag false\"\n \tEOF\n \tgit tag -m nested nested annotated-v4.0 2>actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 793da6e64e..9bcf7c0b40 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -224,7 +224,7 @@ test_expect_success 'switch to another branch while carrying a deletion' '\n '\n \n test_expect_success 'checkout to detach HEAD (with advice declined)' '\n-\tgit config advice.detachedHead false &&\n+\tgit config set advice.detachedHead false &&\n \trev=$(git rev-parse --short renamer^) &&\n \tgit checkout -f renamer &&\n \tgit clean -f &&\n@@ -244,7 +244,7 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '\n '\n \n test_expect_success 'checkout to detach HEAD' '\n-\tgit config advice.detachedHead true &&\n+\tgit config set advice.detachedHead true &&\n \trev=$(git rev-parse --short renamer^) &&\n \tgit checkout -f renamer &&\n \tgit clean -f &&\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 981488885f..d6a501d453 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -212,7 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t\tThe following paths are ignored by one of your .gitignore files:\n \t\tsubmod\n \t\thint: Use -f if you really want to add them.\n-\t\thint: Disable this message with \"git config advice.addIgnoredFile false\"\n+\t\thint: Disable this message with \"git config set advice.addIgnoredFile false\"\n \t\tEOF\n \t\t# Does not use test_commit due to the ignore\n \t\techo \"*\" > .gitignore &&\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex f9a5c98f3f..b2070d4e39 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1699,7 +1699,7 @@ test_expect_success 'setup slow status advice' '\n \t\tEOF\n \t\tgit add .gitignore &&\n \t\tgit commit -m \"Add .gitignore\" &&\n-\t\tgit config advice.statusuoption true\n+\t\tgit config set advice.statusuoption true\n \t)\n '\n \n-- \n2.47.1.398.g18e7475ebe\n\n"},{"id":"508617","messageId":"fsqe37ibvarrsjugc4r2cairndr37cmyc64jneaqzhkq4qiiqd@6rskou37aqat","threadId":"62598","inReplyTo":"20241204130928.1059851-1-bence@ferdinandy.com","subject":"Re: [PATCH] advice: suggest using subcommand \"git config set\"","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-12-04T17:19:24Z","receivedAt":"2024-12-04T17:21:30Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/12/04 02:08PM, Bence Ferdinandy wrote:\n> The advice message currently suggests using \"git config advice...\" to\n> disable advice messages, but since 00bbdde141f we have the \"set\"\n\nWhen referencing an existing commit, I think there is a preference to\nuse the output of:\n\n  $ git show -s --format=reference 00bbdde141f\n  00bbdde141 (builtin/config: introduce \"set\" subcommand, 2024-05-06)\n\n> subcommand for config. Change the disable advice message to use the\n> subcommand instead. Change all uses of \"git config advice\" in the tests\n> to use the subcommand.\n\nBoth \"git config <config> <value>\" and \"git config set <config> <value>\"\nare functionally the same operation. So the motivation for this seems to\nbe to push/promote usage of the new \"set\" subcommand. I find the newer\ninterface to be more intuitive and in line with modern command\ninterfaces so updating the advice turn off messages here seems\nreasonable to me.\n\nThere does appear to be other instances where the the advice turn off\ninstructions are open-coded and thus retain the prior format. This does\nresult in some inconsistency, which may not be a big deal, but maybe it\nwould make sense to also adjust those sites as part of this series as\nalso. Otherwise the changes in this patch look correct.\n\n-Justin\n"},{"id":"508642","messageId":"D63MDD4V1FLQ.SL5FXZ9YS8J6@ferdinandy.com","threadId":"62598","inReplyTo":"fsqe37ibvarrsjugc4r2cairndr37cmyc64jneaqzhkq4qiiqd@6rskou37aqat","subject":"Re: [PATCH] advice: suggest using subcommand \"git config set\"","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-12-05T08:21:32Z","receivedAt":"2024-12-05T08:22:16Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Wed Dec 04, 2024 at 18:19, Justin Tobler <jltobler@gmail.com> wrote:\n> On 24/12/04 02:08PM, Bence Ferdinandy wrote:\n>> The advice message currently suggests using \"git config advice...\" to\n>> disable advice messages, but since 00bbdde141f we have the \"set\"\n>\n> When referencing an existing commit, I think there is a preference to\n> use the output of:\n>\n>   $ git show -s --format=reference 00bbdde141f\n>   00bbdde141 (builtin/config: introduce \"set\" subcommand, 2024-05-06)\n\nAck.\n\n>\n>> subcommand for config. Change the disable advice message to use the\n>> subcommand instead. Change all uses of \"git config advice\" in the tests\n>> to use the subcommand.\n>\n> Both \"git config <config> <value>\" and \"git config set <config> <value>\"\n> are functionally the same operation. So the motivation for this seems to\n> be to push/promote usage of the new \"set\" subcommand. I find the newer\n> interface to be more intuitive and in line with modern command\n> interfaces so updating the advice turn off messages here seems\n> reasonable to me.\n\nYes, that was the motivation, I'll make that explicit in the commit message.\n\n>\n> There does appear to be other instances where the the advice turn off\n> instructions are open-coded and thus retain the prior format. This does\n> result in some inconsistency, which may not be a big deal, but maybe it\n> would make sense to also adjust those sites as part of this series as\n> also. Otherwise the changes in this patch look correct.\n\nFair point. Grepping the .c files yielded three more instances, I'll change\nthose as well.\n\n\nThanks,\nBence\n"},{"id":"508643","messageId":"Z1FkrsQ5tkz1pFUz@pks.im","threadId":"62598","inReplyTo":"D63MDD4V1FLQ.SL5FXZ9YS8J6@ferdinandy.com","subject":"Re: [PATCH] advice: suggest using subcommand \"git config set\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-05T08:30:38Z","receivedAt":"2024-12-05T08:30:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 05, 2024 at 09:21:32AM +0100, Bence Ferdinandy wrote:\n> On Wed Dec 04, 2024 at 18:19, Justin Tobler <jltobler@gmail.com> wrote:\n> > On 24/12/04 02:08PM, Bence Ferdinandy wrote:\n> > There does appear to be other instances where the the advice turn off\n> > instructions are open-coded and thus retain the prior format. This does\n> > result in some inconsistency, which may not be a big deal, but maybe it\n> > would make sense to also adjust those sites as part of this series as\n> > also. Otherwise the changes in this patch look correct.\n> \n> Fair point. Grepping the .c files yielded three more instances, I'll change\n> those as well.\n\nYeah. Overall I think it is fine to do an iterative transition to the\nnew interface. `git config set` is not going to be the only instance\nthat needs changes, but I very much assume that we will have suggestions\nand warnings all over the place that may recommend other modes of the\ncommand like the equivalent of `git config get`. But these don't have to\nall happen in the same commit, or even the same patch series, from my\npoint of view.\n\nThanks for working on this!\n\nPatrick\n"},{"id":"508670","messageId":"20241205122225.1184215-1-bence@ferdinandy.com","threadId":"62598","inReplyTo":"Z1FkrsQ5tkz1pFUz@pks.im","subject":"[PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-12-05T12:21:58Z","receivedAt":"2024-12-05T12:23:01Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"The advice message currently suggests using \"git config advice...\" to\ndisable advice messages, but since\n\n00bbdde141 (builtin/config: introduce \"set\" subcommand, 2024-05-06)\n\nwe have the \"set\" subcommand for config. Since using the subcommand is\nmore in-line with the modern interface, any advice should be promoting\nits usage. Change the disable advice message to use the subcommand\ninstead. Change all uses of \"git config advice\" in the tests to use the\nsubcommand.\n\nSigned-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n---\n\nNotes:\n    For the tests I just indiscriminately ran:\n    sed -i \"s/git config advice\\./git config set advice./\" t[0-9]*.sh\n    \n    v2: - fixed 3 hardcoded \"git config advice\" type messages\n        - made the motiviation more explicit\n\n advice.c                        | 2 +-\n commit.c                        | 2 +-\n hook.c                          | 2 +-\n object-name.c                   | 2 +-\n t/t0018-advice.sh               | 2 +-\n t/t3200-branch.sh               | 2 +-\n t/t3404-rebase-interactive.sh   | 6 +++---\n t/t3501-revert-cherry-pick.sh   | 2 +-\n t/t3507-cherry-pick-conflict.sh | 6 +++---\n t/t3510-cherry-pick-sequence.sh | 2 +-\n t/t3511-cherry-pick-x.sh        | 2 +-\n t/t3602-rm-sparse-checkout.sh   | 2 +-\n t/t3700-add.sh                  | 6 +++---\n t/t3705-add-sparse-checkout.sh  | 2 +-\n t/t7002-mv-sparse-checkout.sh   | 4 ++--\n t/t7004-tag.sh                  | 2 +-\n t/t7201-co.sh                   | 4 ++--\n t/t7400-submodule-basic.sh      | 2 +-\n t/t7508-status.sh               | 2 +-\n 19 files changed, 27 insertions(+), 27 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 6b879d805c..f7a5130c2c 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -93,7 +93,7 @@ static struct {\n \n static const char turn_off_instructions[] =\n N_(\"\\n\"\n-   \"Disable this message with \\\"git config advice.%s false\\\"\");\n+   \"Disable this message with \\\"git config set advice.%s false\\\"\");\n \n static void vadvise(const char *advice, int display_instructions,\n \t\t    const char *key, va_list params)\ndiff --git a/commit.c b/commit.c\nindex cc03a93036..35ab9bead5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -276,7 +276,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n \t\t\t \"to convert the grafts into replace refs.\\n\"\n \t\t\t \"\\n\"\n \t\t\t \"Turn this message off by running\\n\"\n-\t\t\t \"\\\"git config advice.graftFileDeprecated false\\\"\"));\n+\t\t\t \"\\\"git config set advice.graftFileDeprecated false\\\"\"));\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n \t\tstruct commit_graft *graft = read_graft_line(&buf);\ndiff --git a/hook.c b/hook.c\nindex a9320cb0ce..9ddbdee06d 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -39,7 +39,7 @@ const char *find_hook(struct repository *r, const char *name)\n \t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n \t\t\t\t\t \"it's not set as executable.\\n\"\n \t\t\t\t\t \"You can disable this warning with \"\n-\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n+\t\t\t\t\t \"`git config set advice.ignoredHook false`.\"),\n \t\t\t\t       path.buf);\n \t\t\t}\n \t\t}\ndiff --git a/object-name.c b/object-name.c\nindex c892fbe80a..0fa9008b76 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -952,7 +952,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\"\\n\"\n \t\"where \\\"$br\\\" is somehow empty and a 40-hex ref is created. Please\\n\"\n \t\"examine these refs and maybe delete them. Turn this message off by\\n\"\n-\t\"running \\\"git config advice.objectNameWarning false\\\"\");\n+\t\"running \\\"git config set advice.objectNameWarning false\\\"\");\n \tstruct object_id tmp_oid;\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 9a3db02fde..f68e08d0b1 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -10,7 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n test_expect_success 'advice should be printed when config variable is unset' '\n \tcat >expect <<-\\EOF &&\n \thint: This is a piece of advice\n-\thint: Disable this message with \"git config advice.nestedTag false\"\n+\thint: Disable this message with \"git config set advice.nestedTag false\"\n \tEOF\n \ttest-tool advise \"This is a piece of advice\" 2>actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 2295db3dcb..a3a21c54cf 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1696,7 +1696,7 @@ test_expect_success 'errors if given a bad branch name' '\n \tcat <<-\\EOF >expect &&\n \tfatal: '\\''foo..bar'\\'' is not a valid branch name\n \thint: See `man git check-ref-format`\n-\thint: Disable this message with \"git config advice.refSyntax false\"\n+\thint: Disable this message with \"git config set advice.refSyntax false\"\n \tEOF\n \ttest_must_fail git branch foo..bar >actual 2>&1 &&\n \ttest_cmp expect actual\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b11f04eb33..ecfc02062c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2258,20 +2258,20 @@ test_expect_success 'non-merge commands reject merge commits' '\n \terror: ${SQ}pick${SQ} does not accept merge commits\n \thint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to\n \thint: replay the merge, use ${SQ}merge -C${SQ} on the commit.\n-\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\thint: Disable this message with \"git config set advice.rebaseTodoError false\"\n \terror: invalid line 1: pick $oid\n \terror: ${SQ}reword${SQ} does not accept merge commits\n \thint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to\n \thint: replay the merge and reword the commit message, use\n \thint: ${SQ}merge -c${SQ} on the commit\n-\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\thint: Disable this message with \"git config set advice.rebaseTodoError false\"\n \terror: invalid line 2: reword $oid\n \terror: ${SQ}edit${SQ} does not accept merge commits\n \thint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to\n \thint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then\n \thint: ${SQ}break${SQ} to give the control back to you so that you can\n \thint: do ${SQ}git commit --amend && git rebase --continue${SQ}.\n-\thint: Disable this message with \"git config advice.rebaseTodoError false\"\n+\thint: Disable this message with \"git config set advice.rebaseTodoError false\"\n \terror: invalid line 3: edit $oid\n \terror: cannot squash merge commit into another commit\n \terror: invalid line 4: fixup $oid\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 17a9937962..78b03d769d 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -177,7 +177,7 @@ test_expect_success 'advice from failed revert' '\n \thint: You can instead skip this commit with \"git revert --skip\".\n \thint: To abort and get back to the state before \"git revert\",\n \thint: run \"git revert --abort\".\n-\thint: Disable this message with \"git config advice.mergeConflict false\"\n+\thint: Disable this message with \"git config set advice.mergeConflict false\"\n \tEOF\n \ttest_commit --append --no-tag \"double-add dream\" dream dream &&\n \ttest_must_fail git revert HEAD^ 2>actual &&\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex f3947b400a..44596cb1e8 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -34,7 +34,7 @@ test_expect_success setup '\n \tgit commit --allow-empty --allow-empty-message &&\n \tgit tag empty &&\n \tgit checkout main &&\n-\tgit config advice.detachedhead false\n+\tgit config set advice.detachedhead false\n \n '\n \n@@ -60,7 +60,7 @@ test_expect_success 'advice from failed cherry-pick' '\n \thint: You can instead skip this commit with \"git cherry-pick --skip\".\n \thint: To abort and get back to the state before \"git cherry-pick\",\n \thint: run \"git cherry-pick --abort\".\n-\thint: Disable this message with \"git config advice.mergeConflict false\"\n+\thint: Disable this message with \"git config set advice.mergeConflict false\"\n \tEOF\n \ttest_must_fail git cherry-pick picked 2>actual &&\n \n@@ -75,7 +75,7 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \terror: could not apply \\$picked... picked\n \thint: after resolving the conflicts, mark the corrected paths\n \thint: with 'git add <paths>' or 'git rm <paths>'\n-\thint: Disable this message with \\\"git config advice.mergeConflict false\\\"\n+\thint: Disable this message with \\\"git config set advice.mergeConflict false\\\"\n \tEOF\n \ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n \ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 7eb52b12ed..66ff9db270 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -25,7 +25,7 @@ pristine_detach () {\n }\n \n test_expect_success setup '\n-\tgit config advice.detachedhead false &&\n+\tgit config set advice.detachedhead false &&\n \techo unrelated >unrelated &&\n \tgit add unrelated &&\n \ttest_commit initial foo a &&\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 84a587daf3..98ef13f0a3 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -51,7 +51,7 @@ trailing empty lines\n \"\n \n test_expect_success setup '\n-\tgit config advice.detachedhead false &&\n+\tgit config set advice.detachedhead false &&\n \techo unrelated >unrelated &&\n \tgit add unrelated &&\n \ttest_commit initial foo a &&\ndiff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh\nindex 08580fd3dc..02c7acd617 100755\n--- a/t/t3602-rm-sparse-checkout.sh\n+++ b/t/t3602-rm-sparse-checkout.sh\n@@ -20,7 +20,7 @@ test_expect_success 'setup' \"\n \thint: If you intend to update such entries, try one of the following:\n \thint: * Use the --sparse option.\n \thint: * Disable or modify the sparsity rules.\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \n \techo b | cat sparse_error_header - >sparse_entry_b_error &&\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 4c543a1a7e..df580a5806 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -31,7 +31,7 @@ test_expect_success 'Test with no pathspecs' '\n \tcat >expect <<-EOF &&\n \tNothing specified, nothing added.\n \thint: Maybe you wanted to say ${SQ}git add .${SQ}?\n-\thint: Disable this message with \"git config advice.addEmptyPathspec false\"\n+\thint: Disable this message with \"git config set advice.addEmptyPathspec false\"\n \tEOF\n \tgit add 2>actual &&\n \ttest_cmp expect actual\n@@ -375,7 +375,7 @@ test_expect_success '\"git add\" a embedded repository' '\n \t\thint: \tgit rm --cached inner1\n \t\thint:\n \t\thint: See \"git help submodule\" for more information.\n-\t\thint: Disable this message with \"git config advice.addEmbeddedRepo false\"\n+\t\thint: Disable this message with \"git config set advice.addEmbeddedRepo false\"\n \t\twarning: adding embedded git repository: inner2\n \t\tEOF\n \t\ttest_cmp expect actual\n@@ -413,7 +413,7 @@ cat >expect.err <<\\EOF\n The following paths are ignored by one of your .gitignore files:\n ignored-file\n hint: Use -f if you really want to add them.\n-hint: Disable this message with \"git config advice.addIgnoredFile false\"\n+hint: Disable this message with \"git config set advice.addIgnoredFile false\"\n EOF\n cat >expect.out <<\\EOF\n add 'track-this'\ndiff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh\nindex 2bade9e804..53a4782267 100755\n--- a/t/t3705-add-sparse-checkout.sh\n+++ b/t/t3705-add-sparse-checkout.sh\n@@ -54,7 +54,7 @@ test_expect_success 'setup' \"\n \thint: If you intend to update such entries, try one of the following:\n \thint: * Use the --sparse option.\n \thint: * Disable or modify the sparsity rules.\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \n \techo sparse_entry | cat sparse_error_header - >sparse_entry_error &&\ndiff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh\nindex 26582ae4e5..4d3f221224 100755\n--- a/t/t7002-mv-sparse-checkout.sh\n+++ b/t/t7002-mv-sparse-checkout.sh\n@@ -32,7 +32,7 @@ test_expect_success 'setup' \"\n \thint: If you intend to update such entries, try one of the following:\n \thint: * Use the --sparse option.\n \thint: * Disable or modify the sparsity rules.\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \n \tcat >dirty_error_header <<-EOF &&\n@@ -45,7 +45,7 @@ test_expect_success 'setup' \"\n \thint: To correct the sparsity of these paths, do the following:\n \thint: * Use \\\"git add --sparse <paths>\\\" to update the index\n \thint: * Use \\\"git sparse-checkout reapply\\\" to apply the sparsity rules\n-\thint: Disable this message with \\\"git config advice.updateSparsePath false\\\"\n+\thint: Disable this message with \\\"git config set advice.updateSparsePath false\\\"\n \tEOF\n \"\n \ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b1316e62f4..7cd5e16dc8 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1850,7 +1850,7 @@ test_expect_success 'recursive tagging should give advice' '\n \thint: already a tag. If you meant to tag the object that it points to, use:\n \thint:\n \thint: \tgit tag -f nested annotated-v4.0^{}\n-\thint: Disable this message with \"git config advice.nestedTag false\"\n+\thint: Disable this message with \"git config set advice.nestedTag false\"\n \tEOF\n \tgit tag -m nested nested annotated-v4.0 2>actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 793da6e64e..9bcf7c0b40 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -224,7 +224,7 @@ test_expect_success 'switch to another branch while carrying a deletion' '\n '\n \n test_expect_success 'checkout to detach HEAD (with advice declined)' '\n-\tgit config advice.detachedHead false &&\n+\tgit config set advice.detachedHead false &&\n \trev=$(git rev-parse --short renamer^) &&\n \tgit checkout -f renamer &&\n \tgit clean -f &&\n@@ -244,7 +244,7 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '\n '\n \n test_expect_success 'checkout to detach HEAD' '\n-\tgit config advice.detachedHead true &&\n+\tgit config set advice.detachedHead true &&\n \trev=$(git rev-parse --short renamer^) &&\n \tgit checkout -f renamer &&\n \tgit clean -f &&\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 981488885f..d6a501d453 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -212,7 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t\tThe following paths are ignored by one of your .gitignore files:\n \t\tsubmod\n \t\thint: Use -f if you really want to add them.\n-\t\thint: Disable this message with \"git config advice.addIgnoredFile false\"\n+\t\thint: Disable this message with \"git config set advice.addIgnoredFile false\"\n \t\tEOF\n \t\t# Does not use test_commit due to the ignore\n \t\techo \"*\" > .gitignore &&\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex f9a5c98f3f..b2070d4e39 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1699,7 +1699,7 @@ test_expect_success 'setup slow status advice' '\n \t\tEOF\n \t\tgit add .gitignore &&\n \t\tgit commit -m \"Add .gitignore\" &&\n-\t\tgit config advice.statusuoption true\n+\t\tgit config set advice.statusuoption true\n \t)\n '\n \n-- \n2.47.1.398.g18e7475ebe\n\n"},{"id":"508697","messageId":"xmqqplm51rdd.fsf@gitster.g","threadId":"62598","inReplyTo":"Z1FkrsQ5tkz1pFUz@pks.im","subject":"Re: [PATCH] advice: suggest using subcommand \"git config set\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-06T02:23:42Z","receivedAt":"2024-12-06T02:23:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Yeah. Overall I think it is fine to do an iterative transition to the\n> new interface. `git config set` is not going to be the only instance\n> that needs changes, but I very much assume that we will have suggestions\n> and warnings all over the place that may recommend other modes of the\n> command like the equivalent of `git config get`. But these don't have to\n> all happen in the same commit, or even the same patch series, from my\n> point of view.\n>\n> Thanks for working on this!\n\nExactly.  We may have to keep both old and new (more explicit) ways\nto spell the subcommand, and consistently using the new way in our\ndocumentation pages and instruction given in advice messages is a\ngood thing, but it is more or less a clean-up effort we can do at\nleisure ;-)  As long as we make sure we finish before we mark the\nold way deprecated, it is perfectly fine.\n\nThanks.\n"},{"id":"508714","messageId":"Z1K8ZPF9eOqQYsqf@pks.im","threadId":"62598","inReplyTo":"20241205122225.1184215-1-bence@ferdinandy.com","subject":"Re: [PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T08:57:08Z","receivedAt":"2024-12-06T08:57:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 05, 2024 at 01:21:58PM +0100, Bence Ferdinandy wrote:\n> The advice message currently suggests using \"git config advice...\" to\n> disable advice messages, but since\n> \n> 00bbdde141 (builtin/config: introduce \"set\" subcommand, 2024-05-06)\n> \n> we have the \"set\" subcommand for config. Since using the subcommand is\n> more in-line with the modern interface, any advice should be promoting\n> its usage. Change the disable advice message to use the subcommand\n> instead. Change all uses of \"git config advice\" in the tests to use the\n> subcommand.\n\nThanks, this version looks good to me!\n\nPatrick\n"},{"id":"508829","messageId":"0e139151-7162-42b3-afae-248c28bf4c4b@gmail.com","threadId":"62598","inReplyTo":"20241205122225.1184215-1-bence@ferdinandy.com","subject":"Re: [PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-08T08:08:59Z","receivedAt":"2024-12-08T08:09:03Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Dec 05, 2024 at 01:21:58PM +0100, Bence Ferdinandy wrote:\n\n> The advice message currently suggests using \"git config advice...\" to\n> disable advice messages, but since\n> \n> 00bbdde141 (builtin/config: introduce \"set\" subcommand, 2024-05-06)\n> \n> we have the \"set\" subcommand for config. Since using the subcommand is\n> more in-line with the modern interface, any advice should be promoting\n> its usage. Change the disable advice message to use the subcommand\n> instead.\n\nIt's very consistent to keep our messages updated with respect to\nchanges in the user interface.  So this patch is a step in the right\ndirection.  Thanks for working on this.\n\n> Change all uses of \"git config advice\" in the tests to use the\n> subcommand.\n\nMaybe this should be done in a separate patch.\n\n> \n> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n> ---\n> \n> Notes:\n>     For the tests I just indiscriminately ran:\n>     sed -i \"s/git config advice\\./git config set advice./\" t[0-9]*.sh\n>     \n>     v2: - fixed 3 hardcoded \"git config advice\" type messages\n>         - made the motiviation more explicit\n> \n>  advice.c                        | 2 +-\n>  commit.c                        | 2 +-\n>  hook.c                          | 2 +-\n>  object-name.c                   | 2 +-\n>  t/t0018-advice.sh               | 2 +-\n>  t/t3200-branch.sh               | 2 +-\n>  t/t3404-rebase-interactive.sh   | 6 +++---\n>  t/t3501-revert-cherry-pick.sh   | 2 +-\n>  t/t3507-cherry-pick-conflict.sh | 6 +++---\n>  t/t3510-cherry-pick-sequence.sh | 2 +-\n>  t/t3511-cherry-pick-x.sh        | 2 +-\n>  t/t3602-rm-sparse-checkout.sh   | 2 +-\n>  t/t3700-add.sh                  | 6 +++---\n>  t/t3705-add-sparse-checkout.sh  | 2 +-\n>  t/t7002-mv-sparse-checkout.sh   | 4 ++--\n>  t/t7004-tag.sh                  | 2 +-\n>  t/t7201-co.sh                   | 4 ++--\n>  t/t7400-submodule-basic.sh      | 2 +-\n>  t/t7508-status.sh               | 2 +-\n>  19 files changed, 27 insertions(+), 27 deletions(-)\n> \n> diff --git a/advice.c b/advice.c\n> index 6b879d805c..f7a5130c2c 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -93,7 +93,7 @@ static struct {\n>  \n>  static const char turn_off_instructions[] =\n>  N_(\"\\n\"\n> -   \"Disable this message with \\\"git config advice.%s false\\\"\");\n> +   \"Disable this message with \\\"git config set advice.%s false\\\"\");\n\nThe main goal of this patch.  Good.\n\n>  \n>  static void vadvise(const char *advice, int display_instructions,\n>  \t\t    const char *key, va_list params)\n> diff --git a/commit.c b/commit.c\n> index cc03a93036..35ab9bead5 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -276,7 +276,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n>  \t\t\t \"to convert the grafts into replace refs.\\n\"\n>  \t\t\t \"\\n\"\n>  \t\t\t \"Turn this message off by running\\n\"\n> -\t\t\t \"\\\"git config advice.graftFileDeprecated false\\\"\"));\n> +\t\t\t \"\\\"git config set advice.graftFileDeprecated false\\\"\"));\n\nOK.\n\nHowever, instead of solidifying this message, perhaps we could take\nadvantage of `advise_if_enabled()` here.  That way, we simplify the\ncode a bit while we also automatically get the new help message, which\nyou are already adjusting in advice.c.\n\nMore on this below.\n\n>  \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n>  \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n>  \t\tstruct commit_graft *graft = read_graft_line(&buf);\n> diff --git a/hook.c b/hook.c\n> index a9320cb0ce..9ddbdee06d 100644\n> --- a/hook.c\n> +++ b/hook.c\n> @@ -39,7 +39,7 @@ const char *find_hook(struct repository *r, const char *name)\n>  \t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n>  \t\t\t\t\t \"it's not set as executable.\\n\"\n>  \t\t\t\t\t \"You can disable this warning with \"\n> -\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n> +\t\t\t\t\t \"`git config set advice.ignoredHook false`.\"),\n\nThis message is more of a warning than advice.  I don't think we want\nto use the same approach here as above, because:\n\n    hint: The 'foo' hook was ignored because it's not set as executable.\n    hint: Disable this message with [...]\n\nlooks weird.\n\nSo, your change is enough and right.  OK.\n\n>  \t\t\t\t       path.buf);\n>  \t\t\t}\n>  \t\t}\n> diff --git a/object-name.c b/object-name.c\n> index c892fbe80a..0fa9008b76 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -952,7 +952,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n>  \t\"\\n\"\n>  \t\"where \\\"$br\\\" is somehow empty and a 40-hex ref is created. Please\\n\"\n>  \t\"examine these refs and maybe delete them. Turn this message off by\\n\"\n> -\t\"running \\\"git config advice.objectNameWarning false\\\"\");\n> +\t\"running \\\"git config set advice.objectNameWarning false\\\"\");\n\nHere, however, I think we should also switch to `advise_if_enabled()`.\n\n[...]\n\nThe rest of the patch looks good.  I think it's desirable to separate\nthe changes in the advice messages from the uses of \"git config set\"\nin the tests, as I commented at the beginning of this message.  But I\ndon't have a strong opinion on it.\n\nI'll reply to this message with the changes I've suggested about using\n`advise_if_enabled()`.  If you agree with the changes, feel free to\nuse them as you wish.\n\nRubén Justo (3):\n  advice: enhance `detach_advice()` to `detach_advice_if_enabled()`\n  commit: use `advise_if_enabled()` in `read_graft_file()`\n  object-name: advice to avoid refs that resemble hashes\n\n advice.c                            |  8 +++-----\n advice.h                            |  2 +-\n builtin/checkout.c                  |  5 ++---\n builtin/clone.c                     |  3 +--\n commit.c                            | 17 +++++++----------\n object-name.c                       |  9 ++++-----\n t/t1512-rev-parse-disambiguation.sh | 15 ++++++++++++++-\n 7 files changed, 32 insertions(+), 27 deletions(-)\n"},{"id":"508830","messageId":"47cf2e55-364c-49d2-a364-4f1276196071@gmail.com","threadId":"62598","inReplyTo":"0e139151-7162-42b3-afae-248c28bf4c4b@gmail.com","subject":"[PATCH 1/3] advice: enhance `detach_advice()` to `detach_advice_if_enabled()`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-08T08:12:11Z","receivedAt":"2024-12-08T08:12:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We have the `detachedHead` advice since 13be3e31f1 (\"Reword 'detached\nHEAD' notification\", 2010-01-29).\n\nThis advice is shown to the user in the `detach_advice()` function, and\nits only two clients verify beforehand if the advice is desired, in\norder to call the function accordingly.\n\nThe `advise_if_enabled()` API encapsulates some functionality that we\ncan take advantage of:\n\n - Checks if the advice is desired, using `advice_enabled()`.\n\n - Automatically adds help, when needed, on how to disable the\n   advice: \"Turn off this advice by ...\".\n\n - Displays the message consistently with other advise messages,\n   prefixing each line with 'hint:'.\n\nLet's simplify the logic for the clients of `detach_advice()` by\neliminating their need to decide whether to show the advice, bringing\nthat decision into `detach_advice()`.  Also, let's make the it use\n`advice_if_enabled()` to ensure consistency with other advice messages.\n\nTo better reflect the changes in the function let's rename it to\n`detach_advice_if_enabled()`.\n\nFinally, note that we have two tests in t7201 related to this advice:\n\"checkout to detach HEAD (with advice declined)\" and \"checkout a detach\nHEAD\".  They are unaffected by the change we're doing here, so it is\nnot necessary to adjust them in this step.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n advice.c           | 8 +++-----\n advice.h           | 2 +-\n builtin/checkout.c | 5 ++---\n builtin/clone.c    | 3 +--\n 4 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 6b879d805c..399ae58437 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -272,7 +272,7 @@ void advise_on_updating_sparse_paths(struct string_list *pathspec_list)\n \t\t\t    \"* Disable or modify the sparsity rules.\"));\n }\n \n-void detach_advice(const char *new_name)\n+void detach_advice_if_enabled(const char *new_name)\n {\n \tconst char *fmt =\n \t_(\"Note: switching to '%s'.\\n\"\n@@ -288,11 +288,9 @@ void detach_advice(const char *new_name)\n \t\"\\n\"\n \t\"Or undo this operation with:\\n\"\n \t\"\\n\"\n-\t\"  git switch -\\n\"\n-\t\"\\n\"\n-\t\"Turn off this advice by setting config variable advice.detachedHead to false\\n\\n\");\n+\t\"  git switch -\\n\");\n \n-\tfprintf(stderr, fmt, new_name);\n+\tadvise_if_enabled(ADVICE_DETACHED_HEAD, fmt, new_name);\n }\n \n void advise_on_moving_dirty_path(struct string_list *pathspec_list)\ndiff --git a/advice.h b/advice.h\nindex d7466bc0ef..739c5e4987 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -79,7 +79,7 @@ void NORETURN die_resolve_conflict(const char *me);\n void NORETURN die_conclude_merge(void);\n void NORETURN die_ff_impossible(void);\n void advise_on_updating_sparse_paths(struct string_list *pathspec_list);\n-void detach_advice(const char *new_name);\n+void detach_advice_if_enabled(const char *new_name);\n void advise_on_moving_dirty_path(struct string_list *pathspec_list);\n \n #endif /* ADVICE_H */\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex c449558e66..e1366556d7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1009,9 +1009,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tNULL,\n \t\t\t\tREF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);\n \t\tif (!opts->quiet) {\n-\t\t\tif (old_branch_info->path &&\n-\t\t\t    advice_enabled(ADVICE_DETACHED_HEAD) && !opts->force_detach)\n-\t\t\t\tdetach_advice(new_branch_info->name);\n+\t\t\tif (old_branch_info->path && !opts->force_detach)\n+\t\t\t\tdetach_advice_if_enabled(new_branch_info->name);\n \t\t\tdescribe_detached_head(_(\"HEAD is now at\"), new_branch_info->commit);\n \t\t}\n \t} else if (new_branch_info->path) {\t/* Switch branches. */\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 21721db28a..d0c5e89a2a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -752,8 +752,7 @@ static int checkout(int submodule_progress, int filter_submodules,\n \t\treturn 0;\n \t}\n \tif (!strcmp(head, \"HEAD\")) {\n-\t\tif (advice_enabled(ADVICE_DETACHED_HEAD))\n-\t\t\tdetach_advice(oid_to_hex(&oid));\n+\t\tdetach_advice_if_enabled(oid_to_hex(&oid));\n \t\tFREE_AND_NULL(head);\n \t} else {\n \t\tif (!starts_with(head, \"refs/heads/\"))\n-- \n2.47.1.407.gf6b6eee3e5\n"},{"id":"508831","messageId":"ec2a47c1-9bfd-4c80-a495-22154e6c0d24@gmail.com","threadId":"62598","inReplyTo":"0e139151-7162-42b3-afae-248c28bf4c4b@gmail.com","subject":"[PATCH 2/3] commit: use `advise_if_enabled()` in `read_graft_file()`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-08T08:12:26Z","receivedAt":"2024-12-08T08:12:28Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We have a deprecation notice in `read_graft_file()` since f9f99b3f7d\n(Deprecate support for .git/info/grafts, 2018-04-29).\n\nThis deprecation notice is shown using `advice_enabled()` plus\n`advise()`.\n\nLet's use the `advise_if_enabled()` API which combines the\nfunctionality of both APIs and offers some advantages, such as:\nstandardizing the presentation of the help on how to disable the\nadvice.\n\nThe test we have in t6001 \"show advice that grafts are deprecated\"\ndoes not need to be adjusted due to the changes in this step.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n commit.c | 17 +++++++----------\n 1 file changed, 7 insertions(+), 10 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex cc03a93036..8d92bc1044 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -267,16 +267,13 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n \tstruct strbuf buf = STRBUF_INIT;\n \tif (!fp)\n \t\treturn -1;\n-\tif (!no_graft_file_deprecated_advice &&\n-\t    advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))\n-\t\tadvise(_(\"Support for <GIT_DIR>/info/grafts is deprecated\\n\"\n-\t\t\t \"and will be removed in a future Git version.\\n\"\n-\t\t\t \"\\n\"\n-\t\t\t \"Please use \\\"git replace --convert-graft-file\\\"\\n\"\n-\t\t\t \"to convert the grafts into replace refs.\\n\"\n-\t\t\t \"\\n\"\n-\t\t\t \"Turn this message off by running\\n\"\n-\t\t\t \"\\\"git config advice.graftFileDeprecated false\\\"\"));\n+\tif (!no_graft_file_deprecated_advice)\n+\t\tadvise_if_enabled(ADVICE_GRAFT_FILE_DEPRECATED,\n+\t\t\t_(\"Support for <GIT_DIR>/info/grafts is deprecated\\n\"\n+\t\t\t  \"and will be removed in a future Git version.\\n\"\n+\t\t\t  \"\\n\"\n+\t\t\t  \"Please use \\\"git replace --convert-graft-file\\\"\\n\"\n+\t\t\t  \"to convert the grafts into replace refs.\\n\"));\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n \t\tstruct commit_graft *graft = read_graft_line(&buf);\n-- \n2.47.1.407.gf6b6eee3e5\n"},{"id":"508832","messageId":"43a66f17-c910-498a-8faa-f801194e6c8e@gmail.com","threadId":"62598","inReplyTo":"0e139151-7162-42b3-afae-248c28bf4c4b@gmail.com","subject":"[PATCH 3/3] object-name: advice to avoid refs that resemble hashes","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-08T08:12:43Z","receivedAt":"2024-12-08T08:12:46Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"If we detect a reference resembling a hash, we advice the user to\navoid using it and delete it.\n\nLet's use the `advise_if_enabled()` API to display the advice with the\naim of achieving simplicity and consistency in how the advice is\npresented.\n\nWhile we're here, let's add some tests for this advice to gain\nvisibility if we unintentionally make changes about it.\n\nFinally, the change from `const char*` to `const char[]` is to avoid\nproblems with \"-Werror=format-security\".\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n object-name.c                       |  9 ++++-----\n t/t1512-rev-parse-disambiguation.sh | 15 ++++++++++++++-\n 2 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex c892fbe80a..baf5422013 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -943,7 +943,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\t struct object_id *oid, unsigned int flags)\n {\n \tstatic const char *warn_msg = \"refname '%.*s' is ambiguous.\";\n-\tstatic const char *object_name_msg = N_(\n+\tstatic const char object_name_msg[] = N_(\n \t\"Git normally never creates a ref that ends with 40 hex characters\\n\"\n \t\"because it will be ignored when you just specify 40-hex. These refs\\n\"\n \t\"may be created by mistake. For example,\\n\"\n@@ -951,8 +951,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\"  git switch -c $br $(git rev-parse ...)\\n\"\n \t\"\\n\"\n \t\"where \\\"$br\\\" is somehow empty and a 40-hex ref is created. Please\\n\"\n-\t\"examine these refs and maybe delete them. Turn this message off by\\n\"\n-\t\"running \\\"git config advice.objectNameWarning false\\\"\");\n+\t\"examine these refs and maybe delete them.\");\n \tstruct object_id tmp_oid;\n \tchar *real_ref = NULL;\n \tint refs_found = 0;\n@@ -964,8 +963,8 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\trefs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0);\n \t\t\tif (refs_found > 0) {\n \t\t\t\twarning(warn_msg, len, str);\n-\t\t\t\tif (advice_enabled(ADVICE_OBJECT_NAME_WARNING))\n-\t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n+\t\t\t\tadvise_if_enabled(ADVICE_OBJECT_NAME_WARNING,\n+\t\t\t\t\t\t  object_name_msg);\n \t\t\t}\n \t\t\tfree(real_ref);\n \t\t}\ndiff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh\nindex 70f1e0a998..18bf4f0046 100755\n--- a/t/t1512-rev-parse-disambiguation.sh\n+++ b/t/t1512-rev-parse-disambiguation.sh\n@@ -371,13 +371,26 @@ test_expect_success 'rev-parse --disambiguate drops duplicates' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous 40-hex ref (with advice declined)' '\n+\tgit config set advice.objectNameWarning false &&\n+\tTREE=$(git mktree </dev/null) &&\n+\tREF=$(git rev-parse HEAD) &&\n+\tVAL=$(git commit-tree $TREE </dev/null) &&\n+\tgit update-ref refs/heads/$REF $VAL &&\n+\ttest $(git rev-parse $REF 2>err) = $REF &&\n+\tgrep \"refname.*${REF}.*ambiguous\" err &&\n+\ttest_grep ! hint: err\n+'\n+\n test_expect_success 'ambiguous 40-hex ref' '\n+\tgit config unset advice.objectNameWarning &&\n \tTREE=$(git mktree </dev/null) &&\n \tREF=$(git rev-parse HEAD) &&\n \tVAL=$(git commit-tree $TREE </dev/null) &&\n \tgit update-ref refs/heads/$REF $VAL &&\n \ttest $(git rev-parse $REF 2>err) = $REF &&\n-\tgrep \"refname.*${REF}.*ambiguous\" err\n+\tgrep \"refname.*${REF}.*ambiguous\" err &&\n+\ttest_grep hint: err\n '\n \n test_expect_success 'ambiguous short sha1 ref' '\n-- \n2.47.1.407.gf6b6eee3e5\n"},{"id":"508863","messageId":"D674P6875UXA.LXGHCJ9EFE0N@ferdinandy.com","threadId":"62598","inReplyTo":"0e139151-7162-42b3-afae-248c28bf4c4b@gmail.com","subject":"Re: [PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-12-09T11:21:17Z","receivedAt":"2024-12-09T11:21:44Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nThanks for taking a looking and the follow-up patches!\n\nOn Sun Dec 08, 2024 at 09:08, Rubén Justo <rjusto@gmail.com> wrote:\n> On Thu, Dec 05, 2024 at 01:21:58PM +0100, Bence Ferdinandy wrote:\n>\n>> The advice message currently suggests using \"git config advice...\" to\n>> disable advice messages, but since\n>> \n>> 00bbdde141 (builtin/config: introduce \"set\" subcommand, 2024-05-06)\n>> \n>> we have the \"set\" subcommand for config. Since using the subcommand is\n>> more in-line with the modern interface, any advice should be promoting\n>> its usage. Change the disable advice message to use the subcommand\n>> instead.\n>\n> It's very consistent to keep our messages updated with respect to\n> changes in the user interface.  So this patch is a step in the right\n> direction.  Thanks for working on this.\n>\n>> Change all uses of \"git config advice\" in the tests to use the\n>> subcommand.\n>\n> Maybe this should be done in a separate patch.\n\nSo I was a bit lazy here, since sed changed both the expected test outputs and\nthe usage, so that could certainly be split into two patches to be prudent.\n\n>\n>> \n>> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n>> ---\n>> \n>> Notes:\n>>     For the tests I just indiscriminately ran:\n>>     sed -i \"s/git config advice\\./git config set advice./\" t[0-9]*.sh\n>>     \n>>     v2: - fixed 3 hardcoded \"git config advice\" type messages\n>>         - made the motiviation more explicit\n>> \n>>  advice.c                        | 2 +-\n>>  commit.c                        | 2 +-\n>>  hook.c                          | 2 +-\n>>  object-name.c                   | 2 +-\n>>  t/t0018-advice.sh               | 2 +-\n>>  t/t3200-branch.sh               | 2 +-\n>>  t/t3404-rebase-interactive.sh   | 6 +++---\n>>  t/t3501-revert-cherry-pick.sh   | 2 +-\n>>  t/t3507-cherry-pick-conflict.sh | 6 +++---\n>>  t/t3510-cherry-pick-sequence.sh | 2 +-\n>>  t/t3511-cherry-pick-x.sh        | 2 +-\n>>  t/t3602-rm-sparse-checkout.sh   | 2 +-\n>>  t/t3700-add.sh                  | 6 +++---\n>>  t/t3705-add-sparse-checkout.sh  | 2 +-\n>>  t/t7002-mv-sparse-checkout.sh   | 4 ++--\n>>  t/t7004-tag.sh                  | 2 +-\n>>  t/t7201-co.sh                   | 4 ++--\n>>  t/t7400-submodule-basic.sh      | 2 +-\n>>  t/t7508-status.sh               | 2 +-\n>>  19 files changed, 27 insertions(+), 27 deletions(-)\n>> \n>> diff --git a/advice.c b/advice.c\n>> index 6b879d805c..f7a5130c2c 100644\n>> --- a/advice.c\n>> +++ b/advice.c\n>> @@ -93,7 +93,7 @@ static struct {\n>>  \n>>  static const char turn_off_instructions[] =\n>>  N_(\"\\n\"\n>> -   \"Disable this message with \\\"git config advice.%s false\\\"\");\n>> +   \"Disable this message with \\\"git config set advice.%s false\\\"\");\n>\n> The main goal of this patch.  Good.\n>\n>>  \n>>  static void vadvise(const char *advice, int display_instructions,\n>>  \t\t    const char *key, va_list params)\n>> diff --git a/commit.c b/commit.c\n>> index cc03a93036..35ab9bead5 100644\n>> --- a/commit.c\n>> +++ b/commit.c\n>> @@ -276,7 +276,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n>>  \t\t\t \"to convert the grafts into replace refs.\\n\"\n>>  \t\t\t \"\\n\"\n>>  \t\t\t \"Turn this message off by running\\n\"\n>> -\t\t\t \"\\\"git config advice.graftFileDeprecated false\\\"\"));\n>> +\t\t\t \"\\\"git config set advice.graftFileDeprecated false\\\"\"));\n>\n> OK.\n>\n> However, instead of solidifying this message, perhaps we could take\n> advantage of `advise_if_enabled()` here.  That way, we simplify the\n> code a bit while we also automatically get the new help message, which\n> you are already adjusting in advice.c.\n>\n> More on this below.\n>\n>>  \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n>>  \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n>>  \t\tstruct commit_graft *graft = read_graft_line(&buf);\n>> diff --git a/hook.c b/hook.c\n>> index a9320cb0ce..9ddbdee06d 100644\n>> --- a/hook.c\n>> +++ b/hook.c\n>> @@ -39,7 +39,7 @@ const char *find_hook(struct repository *r, const char *name)\n>>  \t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n>>  \t\t\t\t\t \"it's not set as executable.\\n\"\n>>  \t\t\t\t\t \"You can disable this warning with \"\n>> -\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n>> +\t\t\t\t\t \"`git config set advice.ignoredHook false`.\"),\n>\n> This message is more of a warning than advice.  I don't think we want\n> to use the same approach here as above, because:\n>\n>     hint: The 'foo' hook was ignored because it's not set as executable.\n>     hint: Disable this message with [...]\n>\n> looks weird.\n>\n> So, your change is enough and right.  OK.\n>\n>>  \t\t\t\t       path.buf);\n>>  \t\t\t}\n>>  \t\t}\n>> diff --git a/object-name.c b/object-name.c\n>> index c892fbe80a..0fa9008b76 100644\n>> --- a/object-name.c\n>> +++ b/object-name.c\n>> @@ -952,7 +952,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n>>  \t\"\\n\"\n>>  \t\"where \\\"$br\\\" is somehow empty and a 40-hex ref is created. Please\\n\"\n>>  \t\"examine these refs and maybe delete them. Turn this message off by\\n\"\n>> -\t\"running \\\"git config advice.objectNameWarning false\\\"\");\n>> +\t\"running \\\"git config set advice.objectNameWarning false\\\"\");\n>\n> Here, however, I think we should also switch to `advise_if_enabled()`.\n>\n> [...]\n>\n> The rest of the patch looks good.  I think it's desirable to separate\n> the changes in the advice messages from the uses of \"git config set\"\n> in the tests, as I commented at the beginning of this message.  But I\n> don't have a strong opinion on it.\n>\n> I'll reply to this message with the changes I've suggested about using\n> `advise_if_enabled()`.  If you agree with the changes, feel free to\n> use them as you wish.\n\nImho my patch is a \"no-brainer\" in that it doesn't really change anything about\ncode or behaviour, while what you sent does, so I think the best way to go with\nthis would be to first just switch to `config set` with already existing stuff\nand then open up the question of changing them in a more meaningful way. In\ngeneral of course it seems like a good idea to bring advice messages under one\ninterface, but there's more in there that I don't think I could argue for or\nagainst with any confidence.\n\nI'll send a v3 with the test usages changes split out.\n\nThanks,\nBence\n\n"},{"id":"508867","messageId":"D6791Z2QPSUW.1LP269FO886XF@ferdinandy.com","threadId":"62598","inReplyTo":"D674P6875UXA.LXGHCJ9EFE0N@ferdinandy.com","subject":"Re: [PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-12-09T14:46:04Z","receivedAt":"2024-12-09T14:46:52Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Mon Dec 09, 2024 at 12:21, Bence Ferdinandy <bence@ferdinandy.com> wrote:\n>\n> Thanks for taking a looking and the follow-up patches!\n>\n> On Sun Dec 08, 2024 at 09:08, Rubén Justo <rjusto@gmail.com> wrote:\n>> On Thu, Dec 05, 2024 at 01:21:58PM +0100, Bence Ferdinandy wrote:\n>>\n>>> The advice message currently suggests using \"git config advice...\" to\n>>> disable advice messages, but since\n>>> \n>>> 00bbdde141 (builtin/config: introduce \"set\" subcommand, 2024-05-06)\n>>> \n>>> we have the \"set\" subcommand for config. Since using the subcommand is\n>>> more in-line with the modern interface, any advice should be promoting\n>>> its usage. Change the disable advice message to use the subcommand\n>>> instead.\n>>\n>> It's very consistent to keep our messages updated with respect to\n>> changes in the user interface.  So this patch is a step in the right\n>> direction.  Thanks for working on this.\n>>\n>>> Change all uses of \"git config advice\" in the tests to use the\n>>> subcommand.\n>>\n>> Maybe this should be done in a separate patch.\n>\n> So I was a bit lazy here, since sed changed both the expected test outputs and\n> the usage, so that could certainly be split into two patches to be prudent.\n>\n>>\n>>> \n>>> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n>>> ---\n>>> \n>>> Notes:\n>>>     For the tests I just indiscriminately ran:\n>>>     sed -i \"s/git config advice\\./git config set advice./\" t[0-9]*.sh\n>>>     \n>>>     v2: - fixed 3 hardcoded \"git config advice\" type messages\n>>>         - made the motiviation more explicit\n>>> \n>>>  advice.c                        | 2 +-\n>>>  commit.c                        | 2 +-\n>>>  hook.c                          | 2 +-\n>>>  object-name.c                   | 2 +-\n>>>  t/t0018-advice.sh               | 2 +-\n>>>  t/t3200-branch.sh               | 2 +-\n>>>  t/t3404-rebase-interactive.sh   | 6 +++---\n>>>  t/t3501-revert-cherry-pick.sh   | 2 +-\n>>>  t/t3507-cherry-pick-conflict.sh | 6 +++---\n>>>  t/t3510-cherry-pick-sequence.sh | 2 +-\n>>>  t/t3511-cherry-pick-x.sh        | 2 +-\n>>>  t/t3602-rm-sparse-checkout.sh   | 2 +-\n>>>  t/t3700-add.sh                  | 6 +++---\n>>>  t/t3705-add-sparse-checkout.sh  | 2 +-\n>>>  t/t7002-mv-sparse-checkout.sh   | 4 ++--\n>>>  t/t7004-tag.sh                  | 2 +-\n>>>  t/t7201-co.sh                   | 4 ++--\n>>>  t/t7400-submodule-basic.sh      | 2 +-\n>>>  t/t7508-status.sh               | 2 +-\n>>>  19 files changed, 27 insertions(+), 27 deletions(-)\n>>> \n>>> diff --git a/advice.c b/advice.c\n>>> index 6b879d805c..f7a5130c2c 100644\n>>> --- a/advice.c\n>>> +++ b/advice.c\n>>> @@ -93,7 +93,7 @@ static struct {\n>>>  \n>>>  static const char turn_off_instructions[] =\n>>>  N_(\"\\n\"\n>>> -   \"Disable this message with \\\"git config advice.%s false\\\"\");\n>>> +   \"Disable this message with \\\"git config set advice.%s false\\\"\");\n>>\n>> The main goal of this patch.  Good.\n>>\n>>>  \n>>>  static void vadvise(const char *advice, int display_instructions,\n>>>  \t\t    const char *key, va_list params)\n>>> diff --git a/commit.c b/commit.c\n>>> index cc03a93036..35ab9bead5 100644\n>>> --- a/commit.c\n>>> +++ b/commit.c\n>>> @@ -276,7 +276,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)\n>>>  \t\t\t \"to convert the grafts into replace refs.\\n\"\n>>>  \t\t\t \"\\n\"\n>>>  \t\t\t \"Turn this message off by running\\n\"\n>>> -\t\t\t \"\\\"git config advice.graftFileDeprecated false\\\"\"));\n>>> +\t\t\t \"\\\"git config set advice.graftFileDeprecated false\\\"\"));\n>>\n>> OK.\n>>\n>> However, instead of solidifying this message, perhaps we could take\n>> advantage of `advise_if_enabled()` here.  That way, we simplify the\n>> code a bit while we also automatically get the new help message, which\n>> you are already adjusting in advice.c.\n>>\n>> More on this below.\n>>\n>>>  \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n>>>  \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n>>>  \t\tstruct commit_graft *graft = read_graft_line(&buf);\n>>> diff --git a/hook.c b/hook.c\n>>> index a9320cb0ce..9ddbdee06d 100644\n>>> --- a/hook.c\n>>> +++ b/hook.c\n>>> @@ -39,7 +39,7 @@ const char *find_hook(struct repository *r, const char *name)\n>>>  \t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n>>>  \t\t\t\t\t \"it's not set as executable.\\n\"\n>>>  \t\t\t\t\t \"You can disable this warning with \"\n>>> -\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n>>> +\t\t\t\t\t \"`git config set advice.ignoredHook false`.\"),\n>>\n>> This message is more of a warning than advice.  I don't think we want\n>> to use the same approach here as above, because:\n>>\n>>     hint: The 'foo' hook was ignored because it's not set as executable.\n>>     hint: Disable this message with [...]\n>>\n>> looks weird.\n>>\n>> So, your change is enough and right.  OK.\n>>\n>>>  \t\t\t\t       path.buf);\n>>>  \t\t\t}\n>>>  \t\t}\n>>> diff --git a/object-name.c b/object-name.c\n>>> index c892fbe80a..0fa9008b76 100644\n>>> --- a/object-name.c\n>>> +++ b/object-name.c\n>>> @@ -952,7 +952,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n>>>  \t\"\\n\"\n>>>  \t\"where \\\"$br\\\" is somehow empty and a 40-hex ref is created. Please\\n\"\n>>>  \t\"examine these refs and maybe delete them. Turn this message off by\\n\"\n>>> -\t\"running \\\"git config advice.objectNameWarning false\\\"\");\n>>> +\t\"running \\\"git config set advice.objectNameWarning false\\\"\");\n>>\n>> Here, however, I think we should also switch to `advise_if_enabled()`.\n>>\n>> [...]\n>>\n>> The rest of the patch looks good.  I think it's desirable to separate\n>> the changes in the advice messages from the uses of \"git config set\"\n>> in the tests, as I commented at the beginning of this message.  But I\n>> don't have a strong opinion on it.\n>>\n>> I'll reply to this message with the changes I've suggested about using\n>> `advise_if_enabled()`.  If you agree with the changes, feel free to\n>> use them as you wish.\n>\n> Imho my patch is a \"no-brainer\" in that it doesn't really change anything about\n> code or behaviour, while what you sent does, so I think the best way to go with\n> this would be to first just switch to `config set` with already existing stuff\n> and then open up the question of changing them in a more meaningful way. In\n> general of course it seems like a good idea to bring advice messages under one\n> interface, but there's more in there that I don't think I could argue for or\n> against with any confidence.\n>\n> I'll send a v3 with the test usages changes split out.\n>\n> Thanks,\n> Bence\n\nI started to split the commit, but realized that I only updated \"git config\nadvice\\.\" to \"git config set advice.\" in the tests. If I split the around five\ninstances of actually using \"git config advice\" in the code, then it starts to\nmake a lot less sense for why it is only for \"advice\" and not for all the other\nuses of \"git config\" in the tests. So I'm now inclined to think that I either\nleave the patch as is, or simple just remove the parts that are not updating\nexpected test outcomes and leave updating usage of \"git config\" in tests for\na later as it would likely be a larger effort to clean up everything to use\nexplicit set/get. This cleanup would also only make sense if there are plans to\ndeprecate the old implicit setting syntax at some point.\n\nSo should I remove the changes to usage in tests or just leave the patch as is?\n\nBest,\nBence\n\n"},{"id":"508886","messageId":"be4ee78e-12d4-44c2-9f82-4f0db7706fea@gmail.com","threadId":"62598","inReplyTo":"D6791Z2QPSUW.1LP269FO886XF@ferdinandy.com","subject":"Re: [PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-09T20:35:03Z","receivedAt":"2024-12-09T20:35:06Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Dec 09, 2024 at 03:46:04PM +0100, Bence Ferdinandy wrote:\n\n> I started to split the commit, but realized that I only updated \"git config\n> advice\\.\" to \"git config set advice.\" in the tests. If I split the around five\n> instances of actually using \"git config advice\" in the code, then it starts to\n> make a lot less sense for why it is only for \"advice\" and not for all the other\n> uses of \"git config\" in the tests.\n\nIf I understand the intention of this series correctly, the main goal\nis to update the help messages we give to the user on how to disable\nthe advice messages.  I think you have addressed that.\n\nUpdating the tests to use the new UI \"git config set advice\" sounds\nin this series, because it's related to the advice machinery.\n\nUpdating the test suite to use the new \"git config\" UI seems out of\nscope, I think.\n\n> So I'm now inclined to think that I either\n> leave the patch as is, or simple just remove the parts that are not updating\n> expected test outcomes and leave updating usage of \"git config\" in tests for\n> a later as it would likely be a larger effort to clean up everything to use\n> explicit set/get. This cleanup would also only make sense if there are plans to\n> deprecate the old implicit setting syntax at some point.\n> \n> So should I remove the changes to usage in tests or just leave the patch as is?\n\nI don't have a strong opinion on this.  Since my message, Junio has\nmarked this series to be merged to \"next\".  I can be perfectly happy\nwith the patch as is.\n\nOn the other hand, perhaps I could send my patches about\n`advise_if_enabled()`, later, rebuilt on this series once the dust has\nsettled.\n"},{"id":"508956","messageId":"D68QSB8Y9UTD.1IO0EU3X8ZX39@ferdinandy.com","threadId":"62598","inReplyTo":"be4ee78e-12d4-44c2-9f82-4f0db7706fea@gmail.com","subject":"Re: [PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-12-11T08:52:27Z","receivedAt":"2024-12-11T08:53:10Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Mon Dec 09, 2024 at 21:35, Rubén Justo <rjusto@gmail.com> wrote:\n[snip]\n>\n> I don't have a strong opinion on this.  Since my message, Junio has\n> marked this series to be merged to \"next\".  I can be perfectly happy\n> with the patch as is.\n>\n> On the other hand, perhaps I could send my patches about\n> `advise_if_enabled()`, later, rebuilt on this series once the dust has\n> settled.\n\nI think that would be rather worthwhile, since having different mechanism for\ndisplaying advice isn't the best for sure. And it seems the patch has indeed\nmade it to \"next\" recently, so I think it would be safe to rebuild on it now.\n\nThanks,\nBence\n\n\n"},{"id":"508994","messageId":"484a5e81-63fc-4bad-a385-252329287031@gmail.com","threadId":"62598","inReplyTo":"D68QSB8Y9UTD.1IO0EU3X8ZX39@ferdinandy.com","subject":"Re: [PATCH v2] advice: suggest using subcommand \"git config set\"","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-12-11T18:00:40Z","receivedAt":"2024-12-11T18:00:43Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 12/11/24 9:52 AM, Bence Ferdinandy wrote:\n> \n> On Mon Dec 09, 2024 at 21:35, Rubén Justo <rjusto@gmail.com> wrote:\n> [snip]\n>>\n>> I don't have a strong opinion on this.  Since my message, Junio has\n>> marked this series to be merged to \"next\".  I can be perfectly happy\n>> with the patch as is.\n>>\n>> On the other hand, perhaps I could send my patches about\n>> `advise_if_enabled()`, later, rebuilt on this series once the dust has\n>> settled.\n> \n> I think that would be rather worthwhile, since having different mechanism for\n> displaying advice isn't the best for sure.\n\nIndeed.\n\n> And it seems the patch has indeed\n> made it to \"next\" recently, so I think it would be safe to rebuild on it now.\n\nYes.  I'll wait a few days, and then I'll send it.\n\nThanks.\n"}]}