diff mbox series

advice: suggest using subcommand "git config set"

Message ID 20241204130928.1059851-1-bence@ferdinandy.com (mailing list archive)
State Superseded
Headers show
Series advice: suggest using subcommand "git config set" | expand

Commit Message

Bence Ferdinandy Dec. 4, 2024, 1:08 p.m. UTC 
The advice message currently suggests using "git config advice..." to
disable advice messages, but since 00bbdde141f we have the "set"
subcommand for config. Change the disable advice message to use the
subcommand instead. Change all uses of "git config advice" in the tests
to use the subcommand.

Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
---

Notes:
    For the tests I just indiscriminately ran:
    sed -i "s/git config advice\./git config set advice./" t[0-9]*.sh

 advice.c                        | 2 +-
 t/t0018-advice.sh               | 2 +-
 t/t3200-branch.sh               | 2 +-
 t/t3404-rebase-interactive.sh   | 6 +++---
 t/t3501-revert-cherry-pick.sh   | 2 +-
 t/t3507-cherry-pick-conflict.sh | 6 +++---
 t/t3510-cherry-pick-sequence.sh | 2 +-
 t/t3511-cherry-pick-x.sh        | 2 +-
 t/t3602-rm-sparse-checkout.sh   | 2 +-
 t/t3700-add.sh                  | 6 +++---
 t/t3705-add-sparse-checkout.sh  | 2 +-
 t/t7002-mv-sparse-checkout.sh   | 4 ++--
 t/t7004-tag.sh                  | 2 +-
 t/t7201-co.sh                   | 4 ++--
 t/t7400-submodule-basic.sh      | 2 +-
 t/t7508-status.sh               | 2 +-
 16 files changed, 24 insertions(+), 24 deletions(-)

Comments

Justin Tobler Dec. 4, 2024, 5:19 p.m. UTC | #1 
On 24/12/04 02:08PM, Bence Ferdinandy wrote:
> The advice message currently suggests using "git config advice..." to
> disable advice messages, but since 00bbdde141f we have the "set"

When referencing an existing commit, I think there is a preference to
use the output of:

  $ git show -s --format=reference 00bbdde141f
  00bbdde141 (builtin/config: introduce "set" subcommand, 2024-05-06)

> subcommand for config. Change the disable advice message to use the
> subcommand instead. Change all uses of "git config advice" in the tests
> to use the subcommand.

Both "git config <config> <value>" and "git config set <config> <value>"
are functionally the same operation. So the motivation for this seems to
be to push/promote usage of the new "set" subcommand. I find the newer
interface to be more intuitive and in line with modern command
interfaces so updating the advice turn off messages here seems
reasonable to me.

There does appear to be other instances where the the advice turn off
instructions are open-coded and thus retain the prior format. This does
result in some inconsistency, which may not be a big deal, but maybe it
would make sense to also adjust those sites as part of this series as
also. Otherwise the changes in this patch look correct.

-Justin
Bence Ferdinandy Dec. 5, 2024, 8:21 a.m. UTC | #2 
On Wed Dec 04, 2024 at 18:19, Justin Tobler <jltobler@gmail.com> wrote:
> On 24/12/04 02:08PM, Bence Ferdinandy wrote:
>> The advice message currently suggests using "git config advice..." to
>> disable advice messages, but since 00bbdde141f we have the "set"
>
> When referencing an existing commit, I think there is a preference to
> use the output of:
>
>   $ git show -s --format=reference 00bbdde141f
>   00bbdde141 (builtin/config: introduce "set" subcommand, 2024-05-06)

Ack.

>
>> subcommand for config. Change the disable advice message to use the
>> subcommand instead. Change all uses of "git config advice" in the tests
>> to use the subcommand.
>
> Both "git config <config> <value>" and "git config set <config> <value>"
> are functionally the same operation. So the motivation for this seems to
> be to push/promote usage of the new "set" subcommand. I find the newer
> interface to be more intuitive and in line with modern command
> interfaces so updating the advice turn off messages here seems
> reasonable to me.

Yes, that was the motivation, I'll make that explicit in the commit message.

>
> There does appear to be other instances where the the advice turn off
> instructions are open-coded and thus retain the prior format. This does
> result in some inconsistency, which may not be a big deal, but maybe it
> would make sense to also adjust those sites as part of this series as
> also. Otherwise the changes in this patch look correct.

Fair point. Grepping the .c files yielded three more instances, I'll change
those as well.


Thanks,
Bence
Patrick Steinhardt Dec. 5, 2024, 8:30 a.m. UTC | #3 
On Thu, Dec 05, 2024 at 09:21:32AM +0100, Bence Ferdinandy wrote:
> On Wed Dec 04, 2024 at 18:19, Justin Tobler <jltobler@gmail.com> wrote:
> > On 24/12/04 02:08PM, Bence Ferdinandy wrote:
> > There does appear to be other instances where the the advice turn off
> > instructions are open-coded and thus retain the prior format. This does
> > result in some inconsistency, which may not be a big deal, but maybe it
> > would make sense to also adjust those sites as part of this series as
> > also. Otherwise the changes in this patch look correct.
> 
> Fair point. Grepping the .c files yielded three more instances, I'll change
> those as well.

Yeah. Overall I think it is fine to do an iterative transition to the
new interface. `git config set` is not going to be the only instance
that needs changes, but I very much assume that we will have suggestions
and warnings all over the place that may recommend other modes of the
command like the equivalent of `git config get`. But these don't have to
all happen in the same commit, or even the same patch series, from my
point of view.

Thanks for working on this!

Patrick
Junio C Hamano Dec. 6, 2024, 2:23 a.m. UTC | #4 
Patrick Steinhardt <ps@pks.im> writes:

> Yeah. Overall I think it is fine to do an iterative transition to the
> new interface. `git config set` is not going to be the only instance
> that needs changes, but I very much assume that we will have suggestions
> and warnings all over the place that may recommend other modes of the
> command like the equivalent of `git config get`. But these don't have to
> all happen in the same commit, or even the same patch series, from my
> point of view.
>
> Thanks for working on this!

Exactly.  We may have to keep both old and new (more explicit) ways
to spell the subcommand, and consistently using the new way in our
documentation pages and instruction given in advice messages is a
good thing, but it is more or less a clean-up effort we can do at
leisure ;-)  As long as we make sure we finish before we mark the
old way deprecated, it is perfectly fine.

Thanks.
diff mbox series

Patch

diff --git a/advice.c b/advice.c
index 6b879d805c..f7a5130c2c 100644
--- a/advice.c
+++ b/advice.c
@@ -93,7 +93,7 @@  static struct {
 
 static const char turn_off_instructions[] =
 N_("\n"
-   "Disable this message with \"git config advice.%s false\"");
+   "Disable this message with \"git config set advice.%s false\"");
 
 static void vadvise(const char *advice, int display_instructions,
 		    const char *key, va_list params)
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index 9a3db02fde..f68e08d0b1 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -10,7 +10,7 @@  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 test_expect_success 'advice should be printed when config variable is unset' '
 	cat >expect <<-\EOF &&
 	hint: This is a piece of advice
-	hint: Disable this message with "git config advice.nestedTag false"
+	hint: Disable this message with "git config set advice.nestedTag false"
 	EOF
 	test-tool advise "This is a piece of advice" 2>actual &&
 	test_cmp expect actual
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 2295db3dcb..a3a21c54cf 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1696,7 +1696,7 @@  test_expect_success 'errors if given a bad branch name' '
 	cat <<-\EOF >expect &&
 	fatal: '\''foo..bar'\'' is not a valid branch name
 	hint: See `man git check-ref-format`
-	hint: Disable this message with "git config advice.refSyntax false"
+	hint: Disable this message with "git config set advice.refSyntax false"
 	EOF
 	test_must_fail git branch foo..bar >actual 2>&1 &&
 	test_cmp expect actual
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index b11f04eb33..ecfc02062c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2258,20 +2258,20 @@  test_expect_success 'non-merge commands reject merge commits' '
 	error: ${SQ}pick${SQ} does not accept merge commits
 	hint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to
 	hint: replay the merge, use ${SQ}merge -C${SQ} on the commit.
-	hint: Disable this message with "git config advice.rebaseTodoError false"
+	hint: Disable this message with "git config set advice.rebaseTodoError false"
 	error: invalid line 1: pick $oid
 	error: ${SQ}reword${SQ} does not accept merge commits
 	hint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to
 	hint: replay the merge and reword the commit message, use
 	hint: ${SQ}merge -c${SQ} on the commit
-	hint: Disable this message with "git config advice.rebaseTodoError false"
+	hint: Disable this message with "git config set advice.rebaseTodoError false"
 	error: invalid line 2: reword $oid
 	error: ${SQ}edit${SQ} does not accept merge commits
 	hint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to
 	hint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then
 	hint: ${SQ}break${SQ} to give the control back to you so that you can
 	hint: do ${SQ}git commit --amend && git rebase --continue${SQ}.
-	hint: Disable this message with "git config advice.rebaseTodoError false"
+	hint: Disable this message with "git config set advice.rebaseTodoError false"
 	error: invalid line 3: edit $oid
 	error: cannot squash merge commit into another commit
 	error: invalid line 4: fixup $oid
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 17a9937962..78b03d769d 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -177,7 +177,7 @@  test_expect_success 'advice from failed revert' '
 	hint: You can instead skip this commit with "git revert --skip".
 	hint: To abort and get back to the state before "git revert",
 	hint: run "git revert --abort".
-	hint: Disable this message with "git config advice.mergeConflict false"
+	hint: Disable this message with "git config set advice.mergeConflict false"
 	EOF
 	test_commit --append --no-tag "double-add dream" dream dream &&
 	test_must_fail git revert HEAD^ 2>actual &&
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index f3947b400a..44596cb1e8 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -34,7 +34,7 @@  test_expect_success setup '
 	git commit --allow-empty --allow-empty-message &&
 	git tag empty &&
 	git checkout main &&
-	git config advice.detachedhead false
+	git config set advice.detachedhead false
 
 '
 
@@ -60,7 +60,7 @@  test_expect_success 'advice from failed cherry-pick' '
 	hint: You can instead skip this commit with "git cherry-pick --skip".
 	hint: To abort and get back to the state before "git cherry-pick",
 	hint: run "git cherry-pick --abort".
-	hint: Disable this message with "git config advice.mergeConflict false"
+	hint: Disable this message with "git config set advice.mergeConflict false"
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
@@ -75,7 +75,7 @@  test_expect_success 'advice from failed cherry-pick --no-commit' "
 	error: could not apply \$picked... picked
 	hint: after resolving the conflicts, mark the corrected paths
 	hint: with 'git add <paths>' or 'git rm <paths>'
-	hint: Disable this message with \"git config advice.mergeConflict false\"
+	hint: Disable this message with \"git config set advice.mergeConflict false\"
 	EOF
 	test_must_fail git cherry-pick --no-commit picked 2>actual &&
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7eb52b12ed..66ff9db270 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -25,7 +25,7 @@  pristine_detach () {
 }
 
 test_expect_success setup '
-	git config advice.detachedhead false &&
+	git config set advice.detachedhead false &&
 	echo unrelated >unrelated &&
 	git add unrelated &&
 	test_commit initial foo a &&
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 84a587daf3..98ef13f0a3 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -51,7 +51,7 @@  trailing empty lines
 "
 
 test_expect_success setup '
-	git config advice.detachedhead false &&
+	git config set advice.detachedhead false &&
 	echo unrelated >unrelated &&
 	git add unrelated &&
 	test_commit initial foo a &&
diff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh
index 08580fd3dc..02c7acd617 100755
--- a/t/t3602-rm-sparse-checkout.sh
+++ b/t/t3602-rm-sparse-checkout.sh
@@ -20,7 +20,7 @@  test_expect_success 'setup' "
 	hint: If you intend to update such entries, try one of the following:
 	hint: * Use the --sparse option.
 	hint: * Disable or modify the sparsity rules.
-	hint: Disable this message with \"git config advice.updateSparsePath false\"
+	hint: Disable this message with \"git config set advice.updateSparsePath false\"
 	EOF
 
 	echo b | cat sparse_error_header - >sparse_entry_b_error &&
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 4c543a1a7e..df580a5806 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -31,7 +31,7 @@  test_expect_success 'Test with no pathspecs' '
 	cat >expect <<-EOF &&
 	Nothing specified, nothing added.
 	hint: Maybe you wanted to say ${SQ}git add .${SQ}?
-	hint: Disable this message with "git config advice.addEmptyPathspec false"
+	hint: Disable this message with "git config set advice.addEmptyPathspec false"
 	EOF
 	git add 2>actual &&
 	test_cmp expect actual
@@ -375,7 +375,7 @@  test_expect_success '"git add" a embedded repository' '
 		hint: 	git rm --cached inner1
 		hint:
 		hint: See "git help submodule" for more information.
-		hint: Disable this message with "git config advice.addEmbeddedRepo false"
+		hint: Disable this message with "git config set advice.addEmbeddedRepo false"
 		warning: adding embedded git repository: inner2
 		EOF
 		test_cmp expect actual
@@ -413,7 +413,7 @@  cat >expect.err <<\EOF
 The following paths are ignored by one of your .gitignore files:
 ignored-file
 hint: Use -f if you really want to add them.
-hint: Disable this message with "git config advice.addIgnoredFile false"
+hint: Disable this message with "git config set advice.addIgnoredFile false"
 EOF
 cat >expect.out <<\EOF
 add 'track-this'
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index 2bade9e804..53a4782267 100755
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -54,7 +54,7 @@  test_expect_success 'setup' "
 	hint: If you intend to update such entries, try one of the following:
 	hint: * Use the --sparse option.
 	hint: * Disable or modify the sparsity rules.
-	hint: Disable this message with \"git config advice.updateSparsePath false\"
+	hint: Disable this message with \"git config set advice.updateSparsePath false\"
 	EOF
 
 	echo sparse_entry | cat sparse_error_header - >sparse_entry_error &&
diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 26582ae4e5..4d3f221224 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -32,7 +32,7 @@  test_expect_success 'setup' "
 	hint: If you intend to update such entries, try one of the following:
 	hint: * Use the --sparse option.
 	hint: * Disable or modify the sparsity rules.
-	hint: Disable this message with \"git config advice.updateSparsePath false\"
+	hint: Disable this message with \"git config set advice.updateSparsePath false\"
 	EOF
 
 	cat >dirty_error_header <<-EOF &&
@@ -45,7 +45,7 @@  test_expect_success 'setup' "
 	hint: To correct the sparsity of these paths, do the following:
 	hint: * Use \"git add --sparse <paths>\" to update the index
 	hint: * Use \"git sparse-checkout reapply\" to apply the sparsity rules
-	hint: Disable this message with \"git config advice.updateSparsePath false\"
+	hint: Disable this message with \"git config set advice.updateSparsePath false\"
 	EOF
 "
 
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b1316e62f4..7cd5e16dc8 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1850,7 +1850,7 @@  test_expect_success 'recursive tagging should give advice' '
 	hint: already a tag. If you meant to tag the object that it points to, use:
 	hint:
 	hint: 	git tag -f nested annotated-v4.0^{}
-	hint: Disable this message with "git config advice.nestedTag false"
+	hint: Disable this message with "git config set advice.nestedTag false"
 	EOF
 	git tag -m nested nested annotated-v4.0 2>actual &&
 	test_cmp expect actual
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 793da6e64e..9bcf7c0b40 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -224,7 +224,7 @@  test_expect_success 'switch to another branch while carrying a deletion' '
 '
 
 test_expect_success 'checkout to detach HEAD (with advice declined)' '
-	git config advice.detachedHead false &&
+	git config set advice.detachedHead false &&
 	rev=$(git rev-parse --short renamer^) &&
 	git checkout -f renamer &&
 	git clean -f &&
@@ -244,7 +244,7 @@  test_expect_success 'checkout to detach HEAD (with advice declined)' '
 '
 
 test_expect_success 'checkout to detach HEAD' '
-	git config advice.detachedHead true &&
+	git config set advice.detachedHead true &&
 	rev=$(git rev-parse --short renamer^) &&
 	git checkout -f renamer &&
 	git clean -f &&
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 981488885f..d6a501d453 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -212,7 +212,7 @@  test_expect_success 'submodule add to .gitignored path fails' '
 		The following paths are ignored by one of your .gitignore files:
 		submod
 		hint: Use -f if you really want to add them.
-		hint: Disable this message with "git config advice.addIgnoredFile false"
+		hint: Disable this message with "git config set advice.addIgnoredFile false"
 		EOF
 		# Does not use test_commit due to the ignore
 		echo "*" > .gitignore &&
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index f9a5c98f3f..b2070d4e39 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1699,7 +1699,7 @@  test_expect_success 'setup slow status advice' '
 		EOF
 		git add .gitignore &&
 		git commit -m "Add .gitignore" &&
-		git config advice.statusuoption true
+		git config set advice.statusuoption true
 	)
 '