{"thread":{"id":"61356","subject":"[PATCH 0/2] advice: add \"all\" option to disable all hints","startedAt":"2024-04-24T03:59:44Z","lastAt":"2024-05-08T00:40:59Z","messageCount":67,"participants":["James Liu","Patrick Steinhardt","Junio C Hamano","Dragan Simic","Phillip Wood","Eric Sunshine","Jeff King","Rubén Justo","Karthik Nayak","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"493386","messageId":"20240424035857.84583-1-james@jamesliu.io","threadId":"61356","inReplyTo":null,"subject":"[PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-24T03:58:55Z","receivedAt":"2024-04-24T03:59:44Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Hello,\n\nThis patch series adds an \"all\" advice hint type that can be used as a\nconvenience option for disabling all advice hints. This is useful in a\nserver context where advice hints won't be seen by a human, and hints\nthat change over time may cause test failures.\n\nThis value should only be set to \"false\", at which point all hints will\nbe disabled. Individual hints can then be enabled by setting their\nrespective types to \"true\".\n\nThe series also modifies the `advise` test tool so it's able to test the\nnormal and special case branches in the advice_enabled() function, and\nadds a few more test cases to verify behaviour.\n\nCheers,\nJames\n\nJames Liu (2):\n advice: allow advice type to be provided in tests\n advice: add \"all\" option to disable all hints\n\n Documentation/config/advice.txt | 5 +++\n advice.c | 8 +++++\n advice.h | 1 +\n t/helper/test-advise.c | 20 +++++++----\n t/t0018-advice.sh | 63 +++++++++++++++++++++++++++++++--\n 5 files changed, 88 insertions(+), 9 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"493387","messageId":"20240424035857.84583-2-james@jamesliu.io","threadId":"61356","inReplyTo":"20240424035857.84583-1-james@jamesliu.io","subject":"[PATCH 1/2] advice: allow advice type to be provided in tests","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-24T03:58:56Z","receivedAt":"2024-04-24T03:59:47Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"advise_if_enabled() has a special branch to handle\nbackwards-compatibility with the `pushUpdateRejected` and\n`pushNonFastForward` advice types, which went untested.\n\nModify the `test-tool advise` command so the advice type can be changed\nbetween nestedTag (the previous behaviour) and pushUpdateRejected.\n\nSigned-off-by: James Liu \n---\n t/helper/test-advise.c | 20 ++++++++++++++------\n t/t0018-advice.sh | 30 +++++++++++++++++++++++++++---\n 2 files changed, 41 insertions(+), 9 deletions(-)\n\ndiff --git a/t/helper/test-advise.c b/t/helper/test-advise.c\nindex 8a3fd0009a..c18b18e059 100644\n--- a/t/helper/test-advise.c\n+++ b/t/helper/test-advise.c\n@@ -5,18 +5,26 @@\n \n int cmd__advise_if_enabled(int argc, const char **argv)\n {\n-\tif (argc != 2)\n-\t\tdie(\"usage: %s \", argv[0]);\n+\tif (argc != 3)\n+\t\tdie(\"usage: %s nestedTag|pushUpdateRejected \", argv[0]);\n \n \tsetup_git_directory();\n \tgit_config(git_default_config, NULL);\n \n \t/*\n-\t * Any advice type can be used for testing, but NESTED_TAG was\n-\t * selected here and in t0018 where this command is being\n-\t * executed.\n+\t * Any advice type can be used for testing, but ADVICE_NESTED_TAG and\n+\t * ADVICE_PUSH_UPDATE_REJECTED were selected here and used in t0018\n+\t * where this command is being executed.\n+\t *\n+\t * This allows test cases to exercise the normal and special branch\n+\t * within advice_enabled().\n \t */\n-\tadvise_if_enabled(ADVICE_NESTED_TAG, \"%s\", argv[1]);\n+\tif (!strcmp(argv[1], \"nestedTag\"))\n+\t\tadvise_if_enabled(ADVICE_NESTED_TAG, \"%s\", argv[2]);\n+\telse if (!strcmp(argv[1], \"pushUpdateRejected\"))\n+\t\tadvise_if_enabled(ADVICE_PUSH_UPDATE_REJECTED, \"%s\", argv[2]);\n+\telse\n+\t\tdie(\"advice type should be nestedTag|pushUpdateRejected\");\n \n \treturn 0;\n }\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 0dcfb760a2..8010796e1f 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -10,7 +10,16 @@ test_expect_success 'advice should be printed when config variable is unset' '\n \thint: This is a piece of advice\n \thint: Disable this message with \"git config advice.nestedTag false\"\n \tEOF\n-\ttest-tool advise \"This is a piece of advice\" 2>actual &&\n+\ttest-tool advise nestedTag \"This is a piece of advice\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'advice should be printed when advice.pushUpdateRejected and its alias are unset' '\n+\tcat >expect <<-\\EOF &&\n+\thint: This is a piece of advice\n+\thint: Disable this message with \"git config advice.pushUpdateRejected false\"\n+\tEOF\n+\ttest-tool advise pushUpdateRejected \"This is a piece of advice\" 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -19,14 +28,29 @@ test_expect_success 'advice should be printed when config variable is set to tru\n \thint: This is a piece of advice\n \tEOF\n \ttest_config advice.nestedTag true &&\n-\ttest-tool advise \"This is a piece of advice\" 2>actual &&\n+\ttest-tool advise nestedTag \"This is a piece of advice\" 2>actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success 'advice should not be printed when config variable is set to false' '\n \ttest_config advice.nestedTag false &&\n-\ttest-tool advise \"This is a piece of advice\" 2>actual &&\n+\ttest-tool advise nestedTag \"This is a piece of advice\" 2>actual &&\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'advice should not be printed when advice.pushUpdateRejected is unset but its alias is set to false' '\n+\ttest_config advice.pushNonFastForward false &&\n+\ttest-tool advise pushUpdateRejected \"This is a piece of advice\" 2>actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'advice should be printed when advice.pushUpdateRejected is set to true and its alias is unset' '\n+\tcat >expect <<-\\EOF &&\n+\thint: This is a piece of advice\n+\tEOF\n+\ttest_config advice.pushUpdateRejected true &&\n+\ttest-tool advise pushUpdateRejected \"This is a piece of advice\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"493388","messageId":"20240424035857.84583-3-james@jamesliu.io","threadId":"61356","inReplyTo":"20240424035857.84583-1-james@jamesliu.io","subject":"[PATCH 2/2] advice: add \"all\" option to disable all hints","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-24T03:58:57Z","receivedAt":"2024-04-24T03:59:51Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Advice hints must be disabled individually by setting the relevant\nadvice.* variables to false in the Git configuration. For server-side\nusages of Git where hints aren't necessary, it can be cumbersome to\nmaintain configuration to disable all advice hints. This is especially\nthe case if/when new advice hints are added.\n\nAdd a new \"all\" advice variable which acts as a toggle for all advice\ntypes. When this is being used, individual advice hints can be enabled\nby setting their respective configs to true.\n\nSigned-off-by: James Liu \n---\n Documentation/config/advice.txt | 5 +++++\n advice.c | 8 ++++++++\n advice.h | 1 +\n t/t0018-advice.sh | 33 +++++++++++++++++++++++++++++++++\n 4 files changed, 47 insertions(+)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 0e35ae5240..0516a23b6b 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -5,6 +5,11 @@ advice.*::\n \tthat you do not need the help message by setting these to `false`:\n +\n --\n+\tall::\n+\t\tConvenience option that allows all advice hints to be\n+\t\tdisabled when set to false. When false, individual\n+\t\tadvice hints can be enabled by setting them to true.\n+\t\tSetting this to true is a no-op.\n \taddEmbeddedRepo::\n \t\tShown when the user accidentally adds one\n \t\tgit repo inside of another.\ndiff --git a/advice.c b/advice.c\nindex 75111191ad..9f0860f143 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -43,6 +43,7 @@ static struct {\n \tconst char *key;\n \tenum advice_level level;\n } advice_setting[] = {\n+\t[ADVICE_ALL]\t\t\t\t\t= { \"all\" },\n \t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\" },\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\" },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\" },\n@@ -132,6 +133,13 @@ int advice_enabled(enum advice_type type)\n \t\treturn enabled &&\n \t\t advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);\n \n+\t/*\n+\t * We still allow for specific advice hints to be enabled if\n+\t * advice.all == false.\n+\t */\n+\tif (advice_setting[ADVICE_ALL].level == ADVICE_LEVEL_DISABLED)\n+\t\treturn advice_setting[type].level == ADVICE_LEVEL_ENABLED;\n+\n \treturn enabled;\n }\n \ndiff --git a/advice.h b/advice.h\nindex c8d29f97f3..b5ac99a645 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -11,6 +11,7 @@ struct string_list;\n * Call advise_if_enabled to print your advice.\n */\n enum advice_type {\n+\tADVICE_ALL,\n \tADVICE_ADD_EMBEDDED_REPO,\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 8010796e1f..19318cc9bb 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -53,4 +53,37 @@ test_expect_success 'advice should be printed when advice.pushUpdateRejected is\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'advice should not be printed when advice.all is set to false' '\n+\ttest_config advice.all false &&\n+\ttest-tool advise nestedTag \"This is a piece of advice\" 2>actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'advice should not be printed for pushUpdateRejected when advice.all is set to false' '\n+\ttest_config advice.all false &&\n+\ttest-tool advise pushUpdateRejected \"This is a piece of advice\" 2>actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'advice should be printed when advice.all is set to false, but specific advice is set to true' '\n+\tcat >expect <<-\\EOF &&\n+\thint: This is a piece of advice\n+\tEOF\n+\ttest_config advice.all false &&\n+\ttest_config advice.nestedTag true &&\n+\ttest-tool advise nestedTag \"This is a piece of advice\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'advice should be printed when advice.all is set to false, but advice.pushUpdateRejected and its alias are set to true' '\n+\tcat >expect <<-\\EOF &&\n+\thint: This is a piece of advice\n+\tEOF\n+\ttest_config advice.all false &&\n+\ttest_config advice.pushUpdateRejected true &&\n+\ttest_config advice.pushNonFastForward true &&\n+\ttest-tool advise pushUpdateRejected \"This is a piece of advice\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"493389","messageId":"ZiiYmpk4IXg3bcb1@tanuki","threadId":"61356","inReplyTo":"20240424035857.84583-2-james@jamesliu.io","subject":"Re: [PATCH 1/2] advice: allow advice type to be provided in tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-24T05:28:58Z","receivedAt":"2024-04-24T05:29:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 24, 2024 at 01:58:56PM +1000, James Liu wrote:\n> advise_if_enabled() has a special branch to handle\n> backwards-compatibility with the `pushUpdateRejected` and\n> `pushNonFastForward` advice types, which went untested.\n> \n> Modify the `test-tool advise` command so the advice type can be changed\n> between nestedTag (the previous behaviour) and pushUpdateRejected.\n> \n> Signed-off-by: James Liu \n> ---\n> t/helper/test-advise.c | 20 ++++++++++++++------\n> t/t0018-advice.sh | 30 +++++++++++++++++++++++++++---\n> 2 files changed, 41 insertions(+), 9 deletions(-)\n> \n> diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c\n> index 8a3fd0009a..c18b18e059 100644\n> --- a/t/helper/test-advise.c\n> +++ b/t/helper/test-advise.c\n> @@ -5,18 +5,26 @@\n> \n> int cmd__advise_if_enabled(int argc, const char **argv)\n> {\n> -\tif (argc != 2)\n> -\t\tdie(\"usage: %s \", argv[0]);\n> +\tif (argc != 3)\n> +\t\tdie(\"usage: %s nestedTag|pushUpdateRejected \", argv[0]);\n\nWe could retain the old behaviour here so that we don't have to update\nall tests. So in case `argc == 2` we implicitly use `ADVICE_NESTED_TAG`,\nif `argc == 3` we look up the advice passed by the caller. The usage\nwould thus essentially become something like this:\n\n usage: test-advise []\n\n> -\tif (argc != 2)\n> -\t\tdie(\"usage: %s \", argv[0]);\n> +\tif (argc != 3)\n> +\t\tdie(\"usage: %s nestedTag|pushUpdateRejected \", argv[0]);\n\n> \tsetup_git_directory();\n> \tgit_config(git_default_config, NULL);\n> \n> \t/*\n> -\t * Any advice type can be used for testing, but NESTED_TAG was\n> -\t * selected here and in t0018 where this command is being\n> -\t * executed.\n> +\t * Any advice type can be used for testing, but ADVICE_NESTED_TAG and\n> +\t * ADVICE_PUSH_UPDATE_REJECTED were selected here and used in t0018\n> +\t * where this command is being executed.\n> +\t *\n> +\t * This allows test cases to exercise the normal and special branch\n> +\t * within advice_enabled().\n> \t */\n> -\tadvise_if_enabled(ADVICE_NESTED_TAG, \"%s\", argv[1]);\n> +\tif (!strcmp(argv[1], \"nestedTag\"))\n> +\t\tadvise_if_enabled(ADVICE_NESTED_TAG, \"%s\", argv[2]);\n> +\telse if (!strcmp(argv[1], \"pushUpdateRejected\"))\n> +\t\tadvise_if_enabled(ADVICE_PUSH_UPDATE_REJECTED, \"%s\", argv[2]);\n> +\telse\n> +\t\tdie(\"advice type should be nestedTag|pushUpdateRejected\");\n\nInstead of singling out these specific advices, can we maybe expose the\n`advice_setting[]` array and make this generic? We could for example add\na new `lookup_advice_by_name()` function that you pass the advice key,\nwhich then walks through the array to look up the `enum advice_type`.\n\nPatrick\n"},{"id":"493390","messageId":"ZiiYoOKJpqq4XWq-@tanuki","threadId":"61356","inReplyTo":"20240424035857.84583-3-james@jamesliu.io","subject":"Re: [PATCH 2/2] advice: add \"all\" option to disable all hints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-24T05:29:04Z","receivedAt":"2024-04-24T05:29:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 24, 2024 at 01:58:57PM +1000, James Liu wrote:\n[snip]\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -43,6 +43,7 @@ static struct {\n> \tconst char *key;\n> \tenum advice_level level;\n> } advice_setting[] = {\n> +\t[ADVICE_ALL]\t\t\t\t\t= { \"all\" },\n> \t[ADVICE_ADD_EMBEDDED_REPO]\t\t\t= { \"addEmbeddedRepo\" },\n> \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\" },\n> \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\" },\n> @@ -132,6 +133,13 @@ int advice_enabled(enum advice_type type)\n> \t\treturn enabled &&\n> \t\t advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);\n> \n> +\t/*\n> +\t * We still allow for specific advice hints to be enabled if\n> +\t * advice.all == false.\n> +\t */\n> +\tif (advice_setting[ADVICE_ALL].level == ADVICE_LEVEL_DISABLED)\n> +\t\treturn advice_setting[type].level == ADVICE_LEVEL_ENABLED;\n\nMakes sense. When the advice is unset we don't need to handle it at all,\nand if it is enabled we basically want to behave the same as before.\n\n> \treturn enabled;\n> }\n> \n> diff --git a/advice.h b/advice.h\n> index c8d29f97f3..b5ac99a645 100644\n> --- a/advice.h\n> +++ b/advice.h\n> @@ -11,6 +11,7 @@ struct string_list;\n> * Call advise_if_enabled to print your advice.\n> */\n> enum advice_type {\n> +\tADVICE_ALL,\n> \tADVICE_ADD_EMBEDDED_REPO,\n> \tADVICE_ADD_EMPTY_PATHSPEC,\n> \tADVICE_ADD_IGNORED_FILE,\n> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> index 8010796e1f..19318cc9bb 100755\n> --- a/t/t0018-advice.sh\n> +++ b/t/t0018-advice.sh\n> @@ -53,4 +53,37 @@ test_expect_success 'advice should be printed when advice.pushUpdateRejected is\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'advice should not be printed when advice.all is set to false' '\n> +\ttest_config advice.all false &&\n> +\ttest-tool advise nestedTag \"This is a piece of advice\" 2>actual &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n> +test_expect_success 'advice should not be printed for pushUpdateRejected when advice.all is set to false' '\n> +\ttest_config advice.all false &&\n> +\ttest-tool advise pushUpdateRejected \"This is a piece of advice\" 2>actual &&\n> +\ttest_must_be_empty actual\n> +'\n> +\n> +test_expect_success 'advice should be printed when advice.all is set to false, but specific advice is set to true' '\n> +\tcat >expect <<-\\EOF &&\n> +\thint: This is a piece of advice\n> +\tEOF\n> +\ttest_config advice.all false &&\n> +\ttest_config advice.nestedTag true &&\n> +\ttest-tool advise nestedTag \"This is a piece of advice\" 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'advice should be printed when advice.all is set to false, but advice.pushUpdateRejected and its alias are set to true' '\n> +\tcat >expect <<-\\EOF &&\n> +\thint: This is a piece of advice\n> +\tEOF\n> +\ttest_config advice.all false &&\n> +\ttest_config advice.pushUpdateRejected true &&\n> +\ttest_config advice.pushNonFastForward true &&\n> +\ttest-tool advise pushUpdateRejected \"This is a piece of advice\" 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> test_done\n\nDo we actually have to enable both advices here? If not, then this\nshould probably be two separate tests that check whether each of the\nkeys does its thing.\n\nPatrick\n"},{"id":"493391","messageId":"xmqqo79z2s24.fsf@gitster.g","threadId":"61356","inReplyTo":"20240424035857.84583-1-james@jamesliu.io","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-24T06:28:03Z","receivedAt":"2024-04-24T06:28:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Liu writes:\n\n> This patch series adds an \"all\" advice hint type that can be used as a\n> convenience option for disabling all advice hints.\n\nHmph. I thought this was rejected already and not in so distant\npast, but I am not finding a discussion thread in the archive.\n\nThe design to support the advice.* variables to let individual ones\nto be squelched, without allowing \"all\", is very much deliberate.\n\nA user may think they are familiar with all the advices already, but\nwith \"all\", they'll never get a chance to learn new ones added after\nthey set the configuration. Looking from the other direction, a new\nadvice message is a way for us to tell our users something important\nfor them to know. For example, we may plan to improve a high-level\nPorcelain command so that it will eventually error out when given an\ninput that used to be accepted but behaved in a way that newbies\nfelt confusing. In the first step of such a transition, we will\nintroduce a new (and hopefully better) way to achieve what the user\nwants to do, and add an advice message to tell the user, when they\ntrigger the codepath whose behaviour will change in the future, in\nthe old way that will eventually go away.\n\nDo not close that communication channel on us.\n\nThanks.\n\n"},{"id":"493392","messageId":"ZiirKgXQPLmtrwLT@tanuki","threadId":"61356","inReplyTo":"xmqqo79z2s24.fsf@gitster.g","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-24T06:48:10Z","receivedAt":"2024-04-24T06:48:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:\n> James Liu writes:\n> \n> > This patch series adds an \"all\" advice hint type that can be used as a\n> > convenience option for disabling all advice hints.\n> \n> Hmph. I thought this was rejected already and not in so distant\n> past, but I am not finding a discussion thread in the archive.\n> \n> The design to support the advice.* variables to let individual ones\n> to be squelched, without allowing \"all\", is very much deliberate.\n> \n> A user may think they are familiar with all the advices already, but\n> with \"all\", they'll never get a chance to learn new ones added after\n> they set the configuration. Looking from the other direction, a new\n> advice message is a way for us to tell our users something important\n> for them to know. For example, we may plan to improve a high-level\n> Porcelain command so that it will eventually error out when given an\n> input that used to be accepted but behaved in a way that newbies\n> felt confusing. In the first step of such a transition, we will\n> introduce a new (and hopefully better) way to achieve what the user\n> wants to do, and add an advice message to tell the user, when they\n> trigger the codepath whose behaviour will change in the future, in\n> the old way that will eventually go away.\n> \n> Do not close that communication channel on us.\n\nWhile I agree that it might not be a good idea to set it for our users,\nthe usecase mentioned by this patch series is scripting. And here I very\nmuch agree with the sentiment that it makes sense to give an easy knob\nto disable all advice (disclosure: James is part of the Gitaly team at\nGitLab, and that is where this feature comes from, so I am very much\nbiased).\n\nIt has happened multiple times to us that new advices were introduced\nthat then subsequently caused regressions in Gitaly because the output\nof Git commands now looks different. While addressing such breakage is\neasy enough to do, it does add up if we have to run Git with every\nsingle advice that we may hit turned off individually.\n\nNow one could say that we shouldn't execute porcelain tools in our\nscripts because it is kind of expected that their output may change at\nany point in time, and that is certainly true. But the reality is that\nthere aren't always good plumbing alternatives available.\n\nFurthermore, we are often forced to parse fragile error messages from\nsuch porcelain tools because Git doesn't provide a better way to dissect\nerrors. Error codes are mostly meaningless and there is no other data\nchannel available to us than the error message.\n\nThese are problems that run deeper than \"We want to disable advices\",\nand we eventually want to address those over time. But it's a long road\nto get there, and meanwhile disabling all advice would be something that\nhelps us make our scripted uses of Git at least a tiny bit more stable.\n\nPatrick\n"},{"id":"493394","messageId":"6516b832041a1d5020f1297c17419279@manjaro.org","threadId":"61356","inReplyTo":"xmqqo79z2s24.fsf@gitster.g","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-24T07:37:36Z","receivedAt":"2024-04-24T07:37:39Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-24 08:28, Junio C Hamano wrote:\n> James Liu writes:\n> \n>> This patch series adds an \"all\" advice hint type that can be used as a\n>> convenience option for disabling all advice hints.\n> \n> Hmph. I thought this was rejected already and not in so distant\n> past, but I am not finding a discussion thread in the archive.\n> \n> The design to support the advice.* variables to let individual ones\n> to be squelched, without allowing \"all\", is very much deliberate.\n> \n> A user may think they are familiar with all the advices already, but\n> with \"all\", they'll never get a chance to learn new ones added after\n> they set the configuration. Looking from the other direction, a new\n> advice message is a way for us to tell our users something important\n> for them to know. For example, we may plan to improve a high-level\n> Porcelain command so that it will eventually error out when given an\n> input that used to be accepted but behaved in a way that newbies\n> felt confusing. In the first step of such a transition, we will\n> introduce a new (and hopefully better) way to achieve what the user\n> wants to do, and add an advice message to tell the user, when they\n> trigger the codepath whose behaviour will change in the future, in\n> the old way that will eventually go away.\n\nI'd agree with Junio. Having \"advice.all\" is like a chainsaw, very\nuseful, but also quite dangerous if not used properly.\n"},{"id":"493402","messageId":"7d1d2892-d486-4e91-b100-e44c1d76893c@gmail.com","threadId":"61356","inReplyTo":"ZiirKgXQPLmtrwLT@tanuki","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-24T13:52:39Z","receivedAt":"2024-04-24T13:52:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 24/04/2024 07:48, Patrick Steinhardt wrote:\n> On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:\n>> Do not close that communication channel on us.\n> \n> While I agree that it might not be a good idea to set it for our users,\n> the usecase mentioned by this patch series is scripting. And here I very\n> much agree with the sentiment that it makes sense to give an easy knob\n> to disable all advice (disclosure: James is part of the Gitaly team at\n> GitLab, and that is where this feature comes from, so I am very much\n> biased).\n\nMaybe an environment variable would be a better fit for turning advice \noff in scripts?\n\n> It has happened multiple times to us that new advices were introduced\n> that then subsequently caused regressions in Gitaly because the output\n> of Git commands now looks different. While addressing such breakage is\n> easy enough to do, it does add up if we have to run Git with every\n> single advice that we may hit turned off individually.\n\nI'm sure you've considered these suggestions but (a) would it be \npossible for Gitaly to filter out lines beginning with \"hint: \" when it \ncaptures the output of commands and (b) would it be possible to have a \nscript that parses advice_setting in advice.c to find the list of advice \nnames so Gitaly can disable them? I think (a) would still leave some \nadvice output from code that uses advice_enabled() rather than \nadvise_if_enabled() but it should get rid of most of the advice messages.\n\nBest Wishes\n\nPhillip\n\n> Now one could say that we shouldn't execute porcelain tools in our\n> scripts because it is kind of expected that their output may change at\n> any point in time, and that is certainly true. But the reality is that\n> there aren't always good plumbing alternatives available.\n> \n> Furthermore, we are often forced to parse fragile error messages from\n> such porcelain tools because Git doesn't provide a better way to dissect\n> errors. Error codes are mostly meaningless and there is no other data\n> channel available to us than the error message.\n> \n> These are problems that run deeper than \"We want to disable advices\",\n> and we eventually want to address those over time. But it's a long road\n> to get there, and meanwhile disabling all advice would be something that\n> helps us make our scripted uses of Git at least a tiny bit more stable.\n> \n> Patrick\n"},{"id":"493403","messageId":"ZikSNSszRzBz715c@ncase","threadId":"61356","inReplyTo":"7d1d2892-d486-4e91-b100-e44c1d76893c@gmail.com","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-24T14:07:49Z","receivedAt":"2024-04-24T14:07:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote:\n> Hi Patrick\n> \n> On 24/04/2024 07:48, Patrick Steinhardt wrote:\n> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:\n> > > Do not close that communication channel on us.\n> > \n> > While I agree that it might not be a good idea to set it for our users,\n> > the usecase mentioned by this patch series is scripting. And here I very\n> > much agree with the sentiment that it makes sense to give an easy knob\n> > to disable all advice (disclosure: James is part of the Gitaly team at\n> > GitLab, and that is where this feature comes from, so I am very much\n> > biased).\n> \n> Maybe an environment variable would be a better fit for turning advice off\n> in scripts?\n\nSure, an environment variable would work just fine for our purposes. It\nwould probably also address the concern that users may disable all\nadvice and then miss out on some information.\n\n> > It has happened multiple times to us that new advices were introduced\n> > that then subsequently caused regressions in Gitaly because the output\n> > of Git commands now looks different. While addressing such breakage is\n> > easy enough to do, it does add up if we have to run Git with every\n> > single advice that we may hit turned off individually.\n> \n> I'm sure you've considered these suggestions but (a) would it be possible\n> for Gitaly to filter out lines beginning with \"hint: \" when it captures the\n> output of commands and (b) would it be possible to have a script that parses\n> advice_setting in advice.c to find the list of advice names so Gitaly can\n> disable them? I think (a) would still leave some advice output from code\n> that uses advice_enabled() rather than advise_if_enabled() but it should get\n> rid of most of the advice messages.\n\nFiltering out advices would be doable. But we probably wouldn't want to\ndo so unconditionally whenever we execute Git commands. Which would\nbring us back to the point where we have to address these new advices\nwhenever they come up, and it wouldn't be much less verbose than to just\ndisable advices we face.\n\nPatrick\n"},{"id":"493404","messageId":"xmqqfrva3k9j.fsf@gitster.g","threadId":"61356","inReplyTo":"ZiirKgXQPLmtrwLT@tanuki","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-24T14:31:04Z","receivedAt":"2024-04-24T14:31:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Do not close that communication channel on us.\n>\n> While I agree that it might not be a good idea to set it for our users,\n> the usecase mentioned by this patch series is scripting. And here I very\n> much agree with the sentiment that it makes sense to give an easy knob\n> to disable all advice (disclosure: James is part of the Gitaly team at\n> GitLab, and that is where this feature comes from, so I am very much\n> biased).\n\nOf course, as you mention, the kosher way is to use the plumbing\nwhere there is no advice messages. If certain functionalitly is\nlacking in the set of plumbing commands, adding them would benefit\neverybody, not just Gitaly.\n\nIf this is for scripting, make it easy for scripts to use while\nmaking it very very hard for users to trigger in their interactive\nenvironments. A configuration variable, by design, is a very bad\nchoice to do so, as it is \"set it once and forget about it\", which\ndefeats the whole purpose of advice messages. An environment\nvariable is very much in the same category in that you can set it in\n~/.login or ~/.profile and forget about it.\n\nA \"git\" wide command line option, similar to \"--no-pager\" or\n\"--literal-pathspecs\", is probably an acceptable avenue.\n\nThanks.\n"},{"id":"493406","messageId":"xmqq34ra3iyj.fsf@gitster.g","threadId":"61356","inReplyTo":"ZikSNSszRzBz715c@ncase","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-24T14:59:16Z","receivedAt":"2024-04-24T14:59:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Filtering out advices would be doable. But we probably wouldn't want to\n> do so unconditionally whenever we execute Git commands.\n\nCan you elaborate? Would you only sometimes show advice messages\nthat come from \"git\" you invoke? Based on what criteria would it\ndecide whether to filter or not to? Is it purely per location in\nyour program (i.e., \"every time the control flow reaches this call\nto an equivalent of run_command(), we would filter the \"hint:\"\nmessages\")?\n\nIn an invocation where you would not filter, what effect does users'\nsetting of advice.* configuration variables have, and what effect\ndoes a new and unseen kind of advice messages have?\n"},{"id":"493411","messageId":"9befd5382109eae65e1c177ff6f04f60@manjaro.org","threadId":"61356","inReplyTo":"ZikSNSszRzBz715c@ncase","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-24T16:14:43Z","receivedAt":"2024-04-24T16:14:45Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello all,\n\nOn 2024-04-24 16:07, Patrick Steinhardt wrote:\n> On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote:\n>> On 24/04/2024 07:48, Patrick Steinhardt wrote:\n>> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:\n>> > > Do not close that communication channel on us.\n>> >\n>> > While I agree that it might not be a good idea to set it for our users,\n>> > the usecase mentioned by this patch series is scripting. And here I very\n>> > much agree with the sentiment that it makes sense to give an easy knob\n>> > to disable all advice (disclosure: James is part of the Gitaly team at\n>> > GitLab, and that is where this feature comes from, so I am very much\n>> > biased).\n>> \n>> Maybe an environment variable would be a better fit for turning advice \n>> off\n>> in scripts?\n> \n> Sure, an environment variable would work just fine for our purposes. It\n> would probably also address the concern that users may disable all\n> advice and then miss out on some information.\n\nSounds good to me. I'd support the addition of a new environment\nvariable for this purpose, perhaps GIT_NO_ADVICE or similar.\n"},{"id":"493415","messageId":"462de4ec1fb1896fa7f26b3515deca57@manjaro.org","threadId":"61356","inReplyTo":"9befd5382109eae65e1c177ff6f04f60@manjaro.org","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-24T16:21:59Z","receivedAt":"2024-04-24T16:22:01Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-24 18:14, Dragan Simic wrote:\n> On 2024-04-24 16:07, Patrick Steinhardt wrote:\n>> On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote:\n>>> On 24/04/2024 07:48, Patrick Steinhardt wrote:\n>>> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:\n>>> > > Do not close that communication channel on us.\n>>> >\n>>> > While I agree that it might not be a good idea to set it for our users,\n>>> > the usecase mentioned by this patch series is scripting. And here I very\n>>> > much agree with the sentiment that it makes sense to give an easy knob\n>>> > to disable all advice (disclosure: James is part of the Gitaly team at\n>>> > GitLab, and that is where this feature comes from, so I am very much\n>>> > biased).\n>>> \n>>> Maybe an environment variable would be a better fit for turning \n>>> advice off\n>>> in scripts?\n>> \n>> Sure, an environment variable would work just fine for our purposes. \n>> It\n>> would probably also address the concern that users may disable all\n>> advice and then miss out on some information.\n> \n> Sounds good to me. I'd support the addition of a new environment\n> variable for this purpose, perhaps GIT_NO_ADVICE or similar.\n\nActually, after reading Junio's comment, [1] I'd rather support\na new command-line option. This is quite similar to disabling\nthe pager, so a new command-line option would IMHO fit rather well\ninto the grand scheme.\n\n[1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/\n"},{"id":"493462","messageId":"Zin7Q6Xdg7WOBNaY@tanuki","threadId":"61356","inReplyTo":"xmqqfrva3k9j.fsf@gitster.g","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-25T06:42:11Z","receivedAt":"2024-04-25T06:42:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 24, 2024 at 07:31:04AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> Do not close that communication channel on us.\n> >\n> > While I agree that it might not be a good idea to set it for our users,\n> > the usecase mentioned by this patch series is scripting. And here I very\n> > much agree with the sentiment that it makes sense to give an easy knob\n> > to disable all advice (disclosure: James is part of the Gitaly team at\n> > GitLab, and that is where this feature comes from, so I am very much\n> > biased).\n> \n> Of course, as you mention, the kosher way is to use the plumbing\n> where there is no advice messages. If certain functionalitly is\n> lacking in the set of plumbing commands, adding them would benefit\n> everybody, not just Gitaly.\n> \n> If this is for scripting, make it easy for scripts to use while\n> making it very very hard for users to trigger in their interactive\n> environments. A configuration variable, by design, is a very bad\n> choice to do so, as it is \"set it once and forget about it\", which\n> defeats the whole purpose of advice messages. An environment\n> variable is very much in the same category in that you can set it in\n> ~/.login or ~/.profile and forget about it.\n> \n> A \"git\" wide command line option, similar to \"--no-pager\" or\n> \"--literal-pathspecs\", is probably an acceptable avenue.\n\nFair enough. So adding a global config like `--no-advice` it is.\n\nPatrick\n"},{"id":"493463","messageId":"Zin8R_iS8_hiy2JE@tanuki","threadId":"61356","inReplyTo":"xmqq34ra3iyj.fsf@gitster.g","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-25T06:46:31Z","receivedAt":"2024-04-25T06:46:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 24, 2024 at 07:59:16AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Filtering out advices would be doable. But we probably wouldn't want to\n> > do so unconditionally whenever we execute Git commands.\n> \n> Can you elaborate? Would you only sometimes show advice messages\n> that come from \"git\" you invoke? Based on what criteria would it\n> decide whether to filter or not to? Is it purely per location in\n> your program (i.e., \"every time the control flow reaches this call\n> to an equivalent of run_command(), we would filter the \"hint:\"\n> messages\")?\n> \n> In an invocation where you would not filter, what effect does users'\n> setting of advice.* configuration variables have, and what effect\n> does a new and unseen kind of advice messages have?\n\nThe reason here is mostly that I do not know whether this can be rigged.\nI cannot state without a doubt that no command may output \"hint:\" at the\nstart of a line that may _not_ actually be an advice. Think for example\n(and I know this example is dumb because advice goes to stderr, not\nstdout) git-cat-file(1) with a blob that contains a line saying \"hint:\".\n\nPatrick\n"},{"id":"493473","messageId":"xmqqmsphh0vy.fsf@gitster.g","threadId":"61356","inReplyTo":"Zin8R_iS8_hiy2JE@tanuki","subject":"Re: [PATCH 0/2] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-25T16:18:09Z","receivedAt":"2024-04-25T16:18:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> > Filtering out advices would be doable. But we probably wouldn't want to\n>> > do so unconditionally whenever we execute Git commands.\n>> ...\n> The reason here is mostly that I do not know whether this can be rigged.\n> I cannot state without a doubt that no command may output \"hint:\" at the\n> start of a line that may _not_ actually be an advice.\n\nAh, I mistook what you said to be \"we may want to keep the advice\nmessages in certain situations while filtering out others\". \n\nSurely, we may send stuff to the standard error stream that happens\nto begin with \"hint:\" that is not created via advice.c:vadvise().\n\nI do not however offhand know why anybody would want to filter such\nlines out when they are filtering out all advice messages. When we\nemit a line that begins with \"hint:\", I do not think of a case when\nwe do not mean as a \"hint\", i.e., in the same spirit as real advice\nmessages.\n"},{"id":"493576","messageId":"20240429010925.93205-1-james@jamesliu.io","threadId":"61356","inReplyTo":"20240424035857.84583-1-james@jamesliu.io","subject":"[PATCH v2 0/1] advice: add --no-advice global option","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-29T01:09:24Z","receivedAt":"2024-04-29T01:09:34Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Hi,\n\nThis is v2 of the patch series to provide a mechanism for silencing all\nadvice hints. Based on feedback, the change now adds a `--no-advice` CLI\noption which works independently of the individual hint toggles.\n\nJames Liu (1):\n advice: add --no-advice global option\n\n Documentation/git.txt | 5 ++++-\n advice.c | 8 +++++++-\n environment.h | 1 +\n git.c | 6 +++++-\n t/t0018-advice.sh | 20 ++++++++++++++++++++\n 5 files changed, 37 insertions(+), 3 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"493577","messageId":"20240429010925.93205-2-james@jamesliu.io","threadId":"61356","inReplyTo":"20240429010925.93205-1-james@jamesliu.io","subject":"[PATCH v2 1/1] advice: add --no-advice global option","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-29T01:09:25Z","receivedAt":"2024-04-29T01:09:39Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Advice hints must be disabled individually by setting the relevant\nadvice.* variables to false in the Git configuration. For server-side\nand scripted usages of Git where hints aren't necessary, it can be\ncumbersome to maintain configuration to ensure all advice hints are\ndisabled in perpetuity. This is a particular concern in tests, where\nnew or changed hints can result in failed assertions.\n\nAdd a --no-advice global option to disable all advice hints from being\ndisplayed. This is independent of the toggles for individual advice\nhints.\n\nSigned-off-by: James Liu \n---\n Documentation/git.txt | 5 ++++-\n advice.c | 8 +++++++-\n environment.h | 1 +\n git.c | 6 +++++-\n t/t0018-advice.sh | 20 ++++++++++++++++++++\n 5 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 7a1b112a3e..ef1d9dce5d 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n [--git-dir=] [--work-tree=] [--namespace=]\n- [--config-env==] []\n+ [--config-env==] [--no-advice] []\n \n DESCRIPTION\n -----------\n@@ -226,6 +226,9 @@ If you just want to run git as if it was started in `` then use\n \tlinkgit:gitattributes[5]. This is equivalent to setting the\n \t`GIT_ATTR_SOURCE` environment variable.\n \n+--no-advice::\n+\tDisable all advice hints from being printed.\n+\n GIT COMMANDS\n ------------\n \ndiff --git a/advice.c b/advice.c\nindex 75111191ad..f6282c3bde 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -2,6 +2,7 @@\n #include \"advice.h\"\n #include \"config.h\"\n #include \"color.h\"\n+#include \"environment.h\"\n #include \"gettext.h\"\n #include \"help.h\"\n #include \"string-list.h\"\n@@ -126,7 +127,12 @@ void advise(const char *advice, ...)\n \n int advice_enabled(enum advice_type type)\n {\n-\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n+\tint enabled;\n+\n+\tif (getenv(GIT_NO_ADVICE))\n+\t\treturn 0;\n+\n+\tenabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n \n \tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n \t\treturn enabled &&\ndiff --git a/environment.h b/environment.h\nindex 05fd94d7be..30c2684269 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -56,6 +56,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);\n #define GIT_OPTIONAL_LOCKS_ENVIRONMENT \"GIT_OPTIONAL_LOCKS\"\n #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n #define GIT_ATTR_SOURCE_ENVIRONMENT \"GIT_ATTR_SOURCE\"\n+#define GIT_NO_ADVICE \"GIT_NO_ADVICE\"\n \n /*\n * Environment variable used in handshaking the wire protocol.\ndiff --git a/git.c b/git.c\nindex 654d615a18..ffeb832ca9 100644\n--- a/git.c\n+++ b/git.c\n@@ -38,7 +38,7 @@ const char git_usage_string[] =\n \t \" [--exec-path[=]] [--html-path] [--man-path] [--info-path]\\n\"\n \t \" [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\\n\"\n \t \" [--git-dir=] [--work-tree=] [--namespace=]\\n\"\n-\t \" [--config-env==] []\");\n+\t \" [--config-env==] [--no-advice] []\");\n \n const char git_more_info_string[] =\n \tN_(\"'git help -a' and 'git help -g' list available subcommands and some\\n\"\n@@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--no-advice\")) {\n+\t\t\tsetenv(GIT_NO_ADVICE, \"1\", 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 0dcfb760a2..2ce2d4668a 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'advice should not be printed when --no-advice is used' '\n+\tcat << EOF > expect &&\n+On branch master\n+\n+No commits yet\n+\n+Untracked files:\n+\tREADME\n+\n+nothing added to commit but untracked files present\n+EOF\n+\n+\tgit init advice-test &&\n+ test_when_finished \"rm -fr advice-test\" &&\n+ cd advice-test &&\n+ touch README &&\n+ git --no-advice status > ../actual &&\n+ test_cmp ../expect ../actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"493579","messageId":"37512328b1f3db4e8075bdb4beeb8929@manjaro.org","threadId":"61356","inReplyTo":"20240429010925.93205-2-james@jamesliu.io","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-29T04:15:33Z","receivedAt":"2024-04-29T04:15:37Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello James,\n\nPlease see my comments below.\n\nOn 2024-04-29 03:09, James Liu wrote:\n> Advice hints must be disabled individually by setting the relevant\n> advice.* variables to false in the Git configuration. For server-side\n> and scripted usages of Git where hints aren't necessary, it can be\n> cumbersome to maintain configuration to ensure all advice hints are\n> disabled in perpetuity. This is a particular concern in tests, where\n> new or changed hints can result in failed assertions.\n> \n> Add a --no-advice global option to disable all advice hints from being\n> displayed. This is independent of the toggles for individual advice\n> hints.\n> \n> Signed-off-by: James Liu \n> ---\n> Documentation/git.txt | 5 ++++-\n> advice.c | 8 +++++++-\n> environment.h | 1 +\n> git.c | 6 +++++-\n> t/t0018-advice.sh | 20 ++++++++++++++++++++\n> 5 files changed, 37 insertions(+), 3 deletions(-)\n> \n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 7a1b112a3e..ef1d9dce5d 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -13,7 +13,7 @@ SYNOPSIS\n> [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n> [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n> [--git-dir=] [--work-tree=] [--namespace=]\n> - [--config-env==] []\n> + [--config-env==] [--no-advice] []\n\nAfter having a more detailed look at the Git documentation,\nI think that adding support for \"--advice\" at the same time\nwould be the right thing to do. That option would override\nall \"advice.\" options that may exist in the configuration\n(and would override the GIT_NO_ADVICE environment variable,\nif we end up supporting it), similarly to how \"--paginate\"\noverrides all \"pager.\" in the configuration, which would\nbe both consistent and rather useful in some situations.\n\n> DESCRIPTION\n> -----------\n> @@ -226,6 +226,9 @@ If you just want to run git as if it was started\n> in `` then use\n> \tlinkgit:gitattributes[5]. This is equivalent to setting the\n> \t`GIT_ATTR_SOURCE` environment variable.\n> \n> +--no-advice::\n> +\tDisable all advice hints from being printed.\n> +\n> GIT COMMANDS\n> ------------\n> \n> diff --git a/advice.c b/advice.c\n> index 75111191ad..f6282c3bde 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -2,6 +2,7 @@\n> #include \"advice.h\"\n> #include \"config.h\"\n> #include \"color.h\"\n> +#include \"environment.h\"\n> #include \"gettext.h\"\n> #include \"help.h\"\n> #include \"string-list.h\"\n> @@ -126,7 +127,12 @@ void advise(const char *advice, ...)\n> \n> int advice_enabled(enum advice_type type)\n> {\n> -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> +\tint enabled;\n> +\n> +\tif (getenv(GIT_NO_ADVICE))\n> +\t\treturn 0;\n\nHuh, I was under impression that having an environment\nvariable to control this behavior was frowned upon by\nJunio? [1] To me, supporting such a variable would be\na somewhat acceptable risk, [2] but of course it's the\nmaintainer's opinion that matters most.\n\n[1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/\n[2] \nhttps://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/\n\n> +\n> +\tenabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> \n> \tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n> \t\treturn enabled &&\n> diff --git a/environment.h b/environment.h\n> index 05fd94d7be..30c2684269 100644\n> --- a/environment.h\n> +++ b/environment.h\n> @@ -56,6 +56,7 @@ const char *getenv_safe(struct strvec *argv, const\n> char *name);\n> #define GIT_OPTIONAL_LOCKS_ENVIRONMENT \"GIT_OPTIONAL_LOCKS\"\n> #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n> #define GIT_ATTR_SOURCE_ENVIRONMENT \"GIT_ATTR_SOURCE\"\n> +#define GIT_NO_ADVICE \"GIT_NO_ADVICE\"\n\nIf we eventually end up supporting new \"GIT_NO_ADVICE\"\nenvironment variable, which I actually doubt, the new\nmacro above should be named \"GIT_NO_ADVICE_ENVIRONMENT\"\ninstead, for consistency.\n\n> \n> /*\n> * Environment variable used in handshaking the wire protocol.\n> diff --git a/git.c b/git.c\n> index 654d615a18..ffeb832ca9 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -38,7 +38,7 @@ const char git_usage_string[] =\n> \t \" [--exec-path[=]] [--html-path] [--man-path]\n> [--info-path]\\n\"\n> \t \" [-p | --paginate | -P | --no-pager]\n> [--no-replace-objects] [--bare]\\n\"\n> \t \" [--git-dir=] [--work-tree=] \n> [--namespace=]\\n\"\n> -\t \" [--config-env==] []\");\n> +\t \" [--config-env==] [--no-advice]\n\nObviously, additional new configuration option \"--advice\"\nwould be also added here, mutually exclusive with \"--no-advice\",\nand into the source code below.\n\n> []\");\n> \n> const char git_more_info_string[] =\n> \tN_(\"'git help -a' and 'git help -g' list available subcommands and \n> some\\n\"\n> @@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int\n> *argc, int *envchanged)\n> \t\t\tsetenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);\n> \t\t\tif (envchanged)\n> \t\t\t\t*envchanged = 1;\n> +\t\t} else if (!strcmp(cmd, \"--no-advice\")) {\n> +\t\t\tsetenv(GIT_NO_ADVICE, \"1\", 1);\n> +\t\t\tif (envchanged)\n> +\t\t\t\t*envchanged = 1;\n> \t\t} else {\n> \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n> \t\t\tusage(git_usage_string);\n> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> index 0dcfb760a2..2ce2d4668a 100755\n> --- a/t/t0018-advice.sh\n> +++ b/t/t0018-advice.sh\n> @@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed\n> when config variable is set to\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'advice should not be printed when --no-advice is \n> used' '\n> +\tcat << EOF > expect &&\n> +On branch master\n> +\n> +No commits yet\n> +\n> +Untracked files:\n> +\tREADME\n> +\n> +nothing added to commit but untracked files present\n> +EOF\n> +\n> +\tgit init advice-test &&\n> + test_when_finished \"rm -fr advice-test\" &&\n> + cd advice-test &&\n> + touch README &&\n> + git --no-advice status > ../actual &&\n> + test_cmp ../expect ../actual\n> +'\n> +\n> test_done\n\nThere should also be a new test that checks how the new\n\"GIT_NO_ADVICE\" environment variable affects the execution,\nbut I doubt it will eventually be supported.\n\nOf course, an additional test should be added to check the\nmutual exclusivity between the above-proposed \"--advice\"\nand \"--no-advice\" options.\n"},{"id":"493581","messageId":"D0WCCOLSMM9K.24BIGLR6EEJI8@jamesliu.io","threadId":"61356","inReplyTo":"37512328b1f3db4e8075bdb4beeb8929@manjaro.org","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-29T05:01:55Z","receivedAt":"2024-04-29T05:02:01Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Thanks for the feedback Dragan!\n\n> After having a more detailed look at the Git documentation,\n> I think that adding support for \"--advice\" at the same time\n> would be the right thing to do. That option would override\n> all \"advice.\" options that may exist in the configuration\n> (and would override the GIT_NO_ADVICE environment variable,\n> if we end up supporting it), similarly to how \"--paginate\"\n> overrides all \"pager.\" in the configuration, which would\n> be both consistent and rather useful in some situations.\n\nI think this makes sense from a UX consistenty perspective, but I'm not\nsure if we should increase the scope of this patch. It's also much\neasier to re-enable previously silenced advice hints, so I'm unsure\nwhether an --advice option makes sense. We can also avoid the decision\nof what to do if the user supplies --advice and --no-advice\nsimultaneously if we only have one option.\n\n> > diff --git a/advice.c b/advice.c\n> > index 75111191ad..f6282c3bde 100644\n> > --- a/advice.c\n> > +++ b/advice.c\n> > @@ -2,6 +2,7 @@\n> > #include \"advice.h\"\n> > #include \"config.h\"\n> > #include \"color.h\"\n> > +#include \"environment.h\"\n> > #include \"gettext.h\"\n> > #include \"help.h\"\n> > #include \"string-list.h\"\n> > @@ -126,7 +127,12 @@ void advise(const char *advice, ...)\n> > \n> > int advice_enabled(enum advice_type type)\n> > {\n> > -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> > +\tint enabled;\n> > +\n> > +\tif (getenv(GIT_NO_ADVICE))\n> > +\t\treturn 0;\n>\n> Huh, I was under impression that having an environment\n> variable to control this behavior was frowned upon by\n> Junio? [1] To me, supporting such a variable would be\n> a somewhat acceptable risk, [2] but of course it's the\n> maintainer's opinion that matters most.\n>\n> [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/\n> [2] \n> https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/\n\nYou're correct. I saw this pattern for a few of the other global CLI\noptions and followed it. I'm unsure what the best alternative for\npassing this configuration down to the `advice_enabled()` function is.\nI'd appreciate some guidance here.\n\nCheers,\nJames\n"},{"id":"493583","messageId":"809a1452fb18a6c03d9e8ed516d5c9c0@manjaro.org","threadId":"61356","inReplyTo":"D0WCCOLSMM9K.24BIGLR6EEJI8@jamesliu.io","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-29T05:36:04Z","receivedAt":"2024-04-29T05:36:06Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-29 07:01, James Liu wrote:\n> Thanks for the feedback Dragan!\n\nThank you for working on the patch.\n\n>> After having a more detailed look at the Git documentation,\n>> I think that adding support for \"--advice\" at the same time\n>> would be the right thing to do. That option would override\n>> all \"advice.\" options that may exist in the configuration\n>> (and would override the GIT_NO_ADVICE environment variable,\n>> if we end up supporting it), similarly to how \"--paginate\"\n>> overrides all \"pager.\" in the configuration, which would\n>> be both consistent and rather useful in some situations.\n> \n> I think this makes sense from a UX consistenty perspective, but I'm not\n> sure if we should increase the scope of this patch. It's also much\n> easier to re-enable previously silenced advice hints, so I'm unsure\n> whether an --advice option makes sense. We can also avoid the decision\n> of what to do if the user supplies --advice and --no-advice\n> simultaneously if we only have one option.\n\nRegarding what to do if those two options are both supplied,\nit's simple, just error out with an appropriate error message.\nThere are already similar situations in the code, e.g. with\nthe -k and --rfc options for git-format-patch(1).\n"},{"id":"493584","messageId":"4991578233e75f5a9f096b701c4b5efd@manjaro.org","threadId":"61356","inReplyTo":"809a1452fb18a6c03d9e8ed516d5c9c0@manjaro.org","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-29T05:59:52Z","receivedAt":"2024-04-29T05:59:54Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-29 07:36, Dragan Simic wrote:\n> On 2024-04-29 07:01, James Liu wrote:\n>> Thanks for the feedback Dragan!\n> \n> Thank you for working on the patch.\n> \n>>> After having a more detailed look at the Git documentation,\n>>> I think that adding support for \"--advice\" at the same time\n>>> would be the right thing to do. That option would override\n>>> all \"advice.\" options that may exist in the configuration\n>>> (and would override the GIT_NO_ADVICE environment variable,\n>>> if we end up supporting it), similarly to how \"--paginate\"\n>>> overrides all \"pager.\" in the configuration, which would\n>>> be both consistent and rather useful in some situations.\n>> \n>> I think this makes sense from a UX consistenty perspective, but I'm \n>> not\n>> sure if we should increase the scope of this patch. It's also much\n>> easier to re-enable previously silenced advice hints, so I'm unsure\n>> whether an --advice option makes sense. We can also avoid the decision\n>> of what to do if the user supplies --advice and --no-advice\n>> simultaneously if we only have one option.\n> \n> Regarding what to do if those two options are both supplied,\n> it's simple, just error out with an appropriate error message.\n> There are already similar situations in the code, e.g. with\n> the -k and --rfc options for git-format-patch(1).\n\nActually, the -p/--paginate and -P/--no-pager options can\ncurrently be supplied together, which isn't the expected\nbehavior. I'm preparing a patch that will cover this as\na case of the mutual option exclusivity.\n"},{"id":"493585","messageId":"CAPig+cRQVD9ZrvBd=49S6=9WsWWNdUWp1u9pADSv17bh_vJGTQ@mail.gmail.com","threadId":"61356","inReplyTo":"4991578233e75f5a9f096b701c4b5efd@manjaro.org","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-29T06:04:35Z","receivedAt":"2024-04-29T06:04:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 29, 2024 at 2:00 AM Dragan Simic wrote:\n> On 2024-04-29 07:36, Dragan Simic wrote:\n> > Regarding what to do if those two options are both supplied,\n> > it's simple, just error out with an appropriate error message.\n> > There are already similar situations in the code, e.g. with\n> > the -k and --rfc options for git-format-patch(1).\n>\n> Actually, the -p/--paginate and -P/--no-pager options can\n> currently be supplied together, which isn't the expected\n> behavior. I'm preparing a patch that will cover this as\n> a case of the mutual option exclusivity.\n\nPlease don't.\n\n\"Last wins\" is an intentionally-supported mode of operation for many\nGit options. It allows you to override an option which may have been\nsupplied by some other entity/mechanism. For instance, you may have a\nGit alias, say `git mylog`, which employs --paginate, but you want to\navoid pagination on a particular run, so you invoke it as `git mylog\n--no-pager`.\n"},{"id":"493587","messageId":"058934338d2ec673958951ae98d55f87@manjaro.org","threadId":"61356","inReplyTo":"CAPig+cRQVD9ZrvBd=49S6=9WsWWNdUWp1u9pADSv17bh_vJGTQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-29T06:12:03Z","receivedAt":"2024-04-29T06:12:06Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Eric,\n\nOn 2024-04-29 08:04, Eric Sunshine wrote:\n> On Mon, Apr 29, 2024 at 2:00 AM Dragan Simic \n> wrote:\n>> On 2024-04-29 07:36, Dragan Simic wrote:\n>> > Regarding what to do if those two options are both supplied,\n>> > it's simple, just error out with an appropriate error message.\n>> > There are already similar situations in the code, e.g. with\n>> > the -k and --rfc options for git-format-patch(1).\n>> \n>> Actually, the -p/--paginate and -P/--no-pager options can\n>> currently be supplied together, which isn't the expected\n>> behavior. I'm preparing a patch that will cover this as\n>> a case of the mutual option exclusivity.\n> \n> Please don't.\n> \n> \"Last wins\" is an intentionally-supported mode of operation for many\n> Git options. It allows you to override an option which may have been\n> supplied by some other entity/mechanism. For instance, you may have a\n> Git alias, say `git mylog`, which employs --paginate, but you want to\n> avoid pagination on a particular run, so you invoke it as `git mylog\n> --no-pager`.\n\nAh, I see, thanks for pointing this out. Makes perfect sense.\n"},{"id":"493604","messageId":"20240429064025.GA2905826@coredump.intra.peff.net","threadId":"61356","inReplyTo":"D0WCCOLSMM9K.24BIGLR6EEJI8@jamesliu.io","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-29T06:40:25Z","receivedAt":"2024-04-29T06:40:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2024 at 03:01:55PM +1000, James Liu wrote:\n\n> > > int advice_enabled(enum advice_type type)\n> > > {\n> > > -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> > > +\tint enabled;\n> > > +\n> > > +\tif (getenv(GIT_NO_ADVICE))\n> > > +\t\treturn 0;\n> >\n> > Huh, I was under impression that having an environment\n> > variable to control this behavior was frowned upon by\n> > Junio? [1] To me, supporting such a variable would be\n> > a somewhat acceptable risk, [2] but of course it's the\n> > maintainer's opinion that matters most.\n> >\n> > [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/\n> > [2] \n> > https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/\n> \n> You're correct. I saw this pattern for a few of the other global CLI\n> options and followed it. I'm unsure what the best alternative for\n> passing this configuration down to the `advice_enabled()` function is.\n> I'd appreciate some guidance here.\n\nYou need an environment variable if you want the command-line option to\nwork consistently across commands that spawn external processes. E.g.:\n\n git --no-advice fetch --all\n\nis going to spawn fetch sub-processes under the hood. You'd want them to\nrespect --no-advice, too, so we either have to propagate the\ncommand-line option or use the environment. And when you consider an\nexternal script like git-foo that runs a bunch of underlying Git\ncommands, then propagating becomes too cumbersome and error-prone.\n\nYou should use git_env_bool() to avoid the confusing behavior that\nGIT_NO_ADVICE=false still turns off advice. ;)\n\nYou can also drop the \"NO\", which helps avoid awkward double negation.\nFor example, if you do:\n\n if (git_env_bool(\"GIT_ADVICE\", 1))\n\treturn 0;\n\nthen leaving that variable unset will act as if it is set to \"1\", but\nyou can still do GIT_ADVICE=0 to suppress it.\n\nThere are some older variables (e.g., GIT_NO_REPLACE_OBJECTS) that made\nthis mistake and we are stuck with, but I think we should avoid it for\nnewer ones.\n\n-Peff\n"},{"id":"493606","messageId":"c75dc8184b005161f0b9cd0cd84f7ce2@manjaro.org","threadId":"61356","inReplyTo":"20240429064025.GA2905826@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-29T06:55:38Z","receivedAt":"2024-04-29T06:55:40Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Jeff,\n\nOn 2024-04-29 08:40, Jeff King wrote:\n> On Mon, Apr 29, 2024 at 03:01:55PM +1000, James Liu wrote:\n> \n>> > > int advice_enabled(enum advice_type type)\n>> > > {\n>> > > -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n>> > > +\tint enabled;\n>> > > +\n>> > > +\tif (getenv(GIT_NO_ADVICE))\n>> > > +\t\treturn 0;\n>> >\n>> > Huh, I was under impression that having an environment\n>> > variable to control this behavior was frowned upon by\n>> > Junio? [1] To me, supporting such a variable would be\n>> > a somewhat acceptable risk, [2] but of course it's the\n>> > maintainer's opinion that matters most.\n>> >\n>> > [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/\n>> > [2] https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/\n>> \n>> You're correct. I saw this pattern for a few of the other global CLI\n>> options and followed it. I'm unsure what the best alternative for\n>> passing this configuration down to the `advice_enabled()` function is.\n>> I'd appreciate some guidance here.\n> \n> You need an environment variable if you want the command-line option to\n> work consistently across commands that spawn external processes. E.g.:\n> \n> git --no-advice fetch --all\n> \n> is going to spawn fetch sub-processes under the hood. You'd want them \n> to\n> respect --no-advice, too, so we either have to propagate the\n> command-line option or use the environment. And when you consider an\n> external script like git-foo that runs a bunch of underlying Git\n> commands, then propagating becomes too cumbersome and error-prone.\n\nWell described, thanks. Though, I'm afraid Junio isn't going\nto like this new environment variable, but we'll see.\n\n> You should use git_env_bool() to avoid the confusing behavior that\n> GIT_NO_ADVICE=false still turns off advice. ;)\n> \n> You can also drop the \"NO\", which helps avoid awkward double negation.\n> For example, if you do:\n> \n> if (git_env_bool(\"GIT_ADVICE\", 1))\n> \treturn 0;\n> \n> then leaving that variable unset will act as if it is set to \"1\", but\n> you can still do GIT_ADVICE=0 to suppress it.\n> \n> There are some older variables (e.g., GIT_NO_REPLACE_OBJECTS) that made\n> this mistake and we are stuck with, but I think we should avoid it for\n> newer ones.\n\nMakes sense to me.\n"},{"id":"493631","messageId":"xmqqmspc1dqm.fsf@gitster.g","threadId":"61356","inReplyTo":"37512328b1f3db4e8075bdb4beeb8929@manjaro.org","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-29T13:48:33Z","receivedAt":"2024-04-29T13:48:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Huh, I was under impression that having an environment\n> variable to control this behavior was frowned upon by\n> Junio?\n\nIt is frowned upon. It is secondary who frowns upon it ;-)\n\nWe _might_ be able to pass --no-advice to all internal invocations\nof subprocesses, but that is a chore, and if a code path calls a\nsubprocess that is *not* a git program that in turn calls a git\nprogram, such an approach would not work. But an environment\nvariable would.\n\nSo using an environment variable as an internal detail and marking\nit as \"internal use only---do not set or unset it yourself\" is the\nbest we could do, and I do not think I would mind. At that point,\nthose who set the variable themselves are outside those whose\nbreakage we care about.\n"},{"id":"493632","messageId":"xmqqh6fk1dmq.fsf@gitster.g","threadId":"61356","inReplyTo":"20240429064025.GA2905826@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-29T13:50:53Z","receivedAt":"2024-04-29T13:50:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You need an environment variable if you want the command-line option to\n> work consistently across commands that spawn external processes. E.g.:\n> ...\n> commands, then propagating becomes too cumbersome and error-prone.\n\nAh, you've explained this---thanks. As an internal implementation\ndetail, this is inevitable, and I am OK as long as we document it as\nsuch.\n\n\n"},{"id":"493658","messageId":"57b6c818-343a-450a-8183-2802abf1c62f@gmail.com","threadId":"61356","inReplyTo":"20240429010925.93205-2-james@jamesliu.io","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-29T17:05:12Z","receivedAt":"2024-04-29T17:05:15Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Apr 29, 2024 at 11:09:25AM +1000, James Liu wrote:\n\n> int advice_enabled(enum advice_type type)\n> {\n> -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> +\tint enabled;\n> +\n> +\tif (getenv(GIT_NO_ADVICE))\n> +\t\treturn 0;\n> +\n> +\tenabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> \n\nAll hints are set to a default visibility value: \"none\", which means\nimplicitly enabled. Maybe we can get this \"no-advice\" knob by making\nthis default value configurable:\n\ndiff --git a/advice.c b/advice.c\nindex 75111191ad..bc67b99ba7 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,8 @@ enum advice_level {\n ADVICE_LEVEL_ENABLED,\n };\n \n+static enum advice_level advice_default_level;\n+\n static struct {\n const char *key;\n enum advice_level level;\n@@ -126,7 +128,19 @@ void advise(const char *advice, ...)\n \n int advice_enabled(enum advice_type type)\n {\n- int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n+ static int once = 0;\n+ int enabled;\n+\n+ if (!once++) {\n+ const char* level = getenv(\"GIT_ADVICE_LEVEL\");\n+\n+ if (level && !strcmp(level, \"disable\"))\n+ advice_default_level = ADVICE_LEVEL_DISABLED;\n+ }\n+\n+ enabled = (advice_setting[type].level\n+ ? advice_setting[type].level\n+ : advice_default_level) != ADVICE_LEVEL_DISABLED;\n \n if (type == ADVICE_PUSH_UPDATE_REJECTED)\n return enabled &&\n\n"},{"id":"493666","messageId":"d2d3e3b6b428e7384b701efba47c5dc1@manjaro.org","threadId":"61356","inReplyTo":"57b6c818-343a-450a-8183-2802abf1c62f@gmail.com","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-29T17:54:55Z","receivedAt":"2024-04-29T17:54:57Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"\n\nOn 2024-04-29 19:05, Rubén Justo wrote:\n> On Mon, Apr 29, 2024 at 11:09:25AM +1000, James Liu wrote:\n> \n>> int advice_enabled(enum advice_type type)\n>> {\n>> -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n>> +\tint enabled;\n>> +\n>> +\tif (getenv(GIT_NO_ADVICE))\n>> +\t\treturn 0;\n>> +\n>> +\tenabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n>> \n> \n> All hints are set to a default visibility value: \"none\", which means\n> implicitly enabled. Maybe we can get this \"no-advice\" knob by making\n> this default value configurable:\n\nPlease note that the new environment variable isn't supposed\nto be used externally, [1] i.e. it isn't meant to be set by\nthe users in their ~/.bash_profile files (or any other shell\nconfiguration files), so I think that extending the scope of\nthis patch into such a direction isn't something we should\naim at.\n\n[1] https://lore.kernel.org/git/xmqqh6fk1dmq.fsf@gitster.g/\n\n> diff --git a/advice.c b/advice.c\n> index 75111191ad..bc67b99ba7 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -39,6 +39,8 @@ enum advice_level {\n> ADVICE_LEVEL_ENABLED,\n> };\n> \n> +static enum advice_level advice_default_level;\n> +\n> static struct {\n> const char *key;\n> enum advice_level level;\n> @@ -126,7 +128,19 @@ void advise(const char *advice, ...)\n> \n> int advice_enabled(enum advice_type type)\n> {\n> - int enabled = advice_setting[type].level != \n> ADVICE_LEVEL_DISABLED;\n> + static int once = 0;\n> + int enabled;\n> +\n> + if (!once++) {\n> + const char* level = getenv(\"GIT_ADVICE_LEVEL\");\n> +\n> + if (level && !strcmp(level, \"disable\"))\n> + advice_default_level = ADVICE_LEVEL_DISABLED;\n> + }\n> +\n> + enabled = (advice_setting[type].level\n> + ? advice_setting[type].level\n> + : advice_default_level) != ADVICE_LEVEL_DISABLED;\n> \n> if (type == ADVICE_PUSH_UPDATE_REJECTED)\n> return enabled &&\n"},{"id":"493716","messageId":"D0X1RH1PK67K.34KDYBQM6XDDU@jamesliu.io","threadId":"61356","inReplyTo":"20240429064025.GA2905826@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/1] advice: add --no-advice global option","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-30T00:56:42Z","receivedAt":"2024-04-30T00:56:48Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Mon Apr 29, 2024 at 4:40 PM AEST, Jeff King wrote:\n> You need an environment variable if you want the command-line option to\n> work consistently across commands that spawn external processes. E.g.:\n>\n> git --no-advice fetch --all\n>\n> is going to spawn fetch sub-processes under the hood. You'd want them to\n> respect --no-advice, too, so we either have to propagate the\n> command-line option or use the environment. And when you consider an\n> external script like git-foo that runs a bunch of underlying Git\n> commands, then propagating becomes too cumbersome and error-prone.\n\nThanks for the explanation Jeff! Makes sense why the pattern is so\nprevalent.\n\n> You should use git_env_bool() to avoid the confusing behavior that\n> GIT_NO_ADVICE=false still turns off advice. ;)\n>\n> You can also drop the \"NO\", which helps avoid awkward double negation.\n> For example, if you do:\n>\n> if (git_env_bool(\"GIT_ADVICE\", 1))\n> \treturn 0;\n>\n> then leaving that variable unset will act as if it is set to \"1\", but\n> you can still do GIT_ADVICE=0 to suppress it.\n\nAwesome. I'll apply this suggestion in the next version of this patch.\n\nCheers,\nJames\n"},{"id":"493717","messageId":"20240430014724.83813-1-james@jamesliu.io","threadId":"61356","inReplyTo":"20240429010925.93205-1-james@jamesliu.io","subject":"[PATCH v3 0/1] advice: add --no-advice global option","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-30T01:47:23Z","receivedAt":"2024-04-30T01:47:31Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Hi,\n\nThis is v3 of the patch to add a global --no-advice option for silencing\nall advice hints. The environment variable has been renamed to\nGIT_ADVICE and marked for internal use only, and the conditional in the\nadvice_enabled() helper has been adjusted to use git_env_bool().\n\nI explored the idea of adding another test to ensure the configuration\nis propagated to subprocesses correctly. This would use `git fetch --all`\nas the trigger, however it appears that advice is only printed when\n`fetch_one()` is invoked, which I don't think spawns any child\nprocesses. With that said, since this is a common pattern, I believe\nthe existing additional test case is sufficient.\n\nCheers,\nJames\n\nJames Liu (1):\n advice: add --no-advice global option\n\n Documentation/git.txt | 5 ++++-\n advice.c | 8 +++++++-\n environment.h | 7 +++++++\n git.c | 6 +++++-\n t/t0018-advice.sh | 20 ++++++++++++++++++++\n 5 files changed, 43 insertions(+), 3 deletions(-)\n\nRange-diff against v2:\n1: 0f2ecb7862 ! 1: 55d5559586 advice: add --no-advice global option\n @@ Commit message\n \n Add a --no-advice global option to disable all advice hints from being\n displayed. This is independent of the toggles for individual advice\n - hints.\n + hints. Use an internal environment variable (GIT_ADVICE) to ensure this\n + configuration is propagated to the usage site, even if it executes in a\n + subprocess.\n \n Signed-off-by: James Liu \n \n @@ advice.c: void advise(const char *advice, ...)\n -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n +\tint enabled;\n +\n -+\tif (getenv(GIT_NO_ADVICE))\n ++\tif (!git_env_bool(GIT_ADVICE, 1))\n +\t\treturn 0;\n +\n +\tenabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n @@ advice.c: void advise(const char *advice, ...)\n \n ## environment.h ##\n @@ environment.h: const char *getenv_safe(struct strvec *argv, const char *name);\n - #define GIT_OPTIONAL_LOCKS_ENVIRONMENT \"GIT_OPTIONAL_LOCKS\"\n #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n #define GIT_ATTR_SOURCE_ENVIRONMENT \"GIT_ATTR_SOURCE\"\n -+#define GIT_NO_ADVICE \"GIT_NO_ADVICE\"\n \n ++/*\n ++ * Environment variable used to propagate the --no-advice global option to the\n ++ * advice_enabled() helper, even when run in a subprocess.\n ++ * This is an internal variable that should not be set by the user.\n ++ */\n ++#define GIT_ADVICE \"GIT_ADVICE\"\n ++\n /*\n * Environment variable used in handshaking the wire protocol.\n + * Contains a colon ':' separated list of keys with optional values\n \n ## git.c ##\n @@ git.c: const char git_usage_string[] =\n @@ git.c: static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n +\t\t} else if (!strcmp(cmd, \"--no-advice\")) {\n -+\t\t\tsetenv(GIT_NO_ADVICE, \"1\", 1);\n ++\t\t\tsetenv(GIT_ADVICE, \"0\", 1);\n +\t\t\tif (envchanged)\n +\t\t\t\t*envchanged = 1;\n \t\t} else {\n-- \n2.44.0\n\n"},{"id":"493718","messageId":"20240430014724.83813-2-james@jamesliu.io","threadId":"61356","inReplyTo":"20240430014724.83813-1-james@jamesliu.io","subject":"[PATCH v3 1/1] advice: add --no-advice global option","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-04-30T01:47:24Z","receivedAt":"2024-04-30T01:47:34Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Advice hints must be disabled individually by setting the relevant\nadvice.* variables to false in the Git configuration. For server-side\nand scripted usages of Git where hints aren't necessary, it can be\ncumbersome to maintain configuration to ensure all advice hints are\ndisabled in perpetuity. This is a particular concern in tests, where\nnew or changed hints can result in failed assertions.\n\nAdd a --no-advice global option to disable all advice hints from being\ndisplayed. This is independent of the toggles for individual advice\nhints. Use an internal environment variable (GIT_ADVICE) to ensure this\nconfiguration is propagated to the usage site, even if it executes in a\nsubprocess.\n\nSigned-off-by: James Liu \n---\n Documentation/git.txt | 5 ++++-\n advice.c | 8 +++++++-\n environment.h | 7 +++++++\n git.c | 6 +++++-\n t/t0018-advice.sh | 20 ++++++++++++++++++++\n 5 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 7a1b112a3e..ef1d9dce5d 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n [--git-dir=] [--work-tree=] [--namespace=]\n- [--config-env==] []\n+ [--config-env==] [--no-advice] []\n \n DESCRIPTION\n -----------\n@@ -226,6 +226,9 @@ If you just want to run git as if it was started in `` then use\n \tlinkgit:gitattributes[5]. This is equivalent to setting the\n \t`GIT_ATTR_SOURCE` environment variable.\n \n+--no-advice::\n+\tDisable all advice hints from being printed.\n+\n GIT COMMANDS\n ------------\n \ndiff --git a/advice.c b/advice.c\nindex 75111191ad..abad9ccaa2 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -2,6 +2,7 @@\n #include \"advice.h\"\n #include \"config.h\"\n #include \"color.h\"\n+#include \"environment.h\"\n #include \"gettext.h\"\n #include \"help.h\"\n #include \"string-list.h\"\n@@ -126,7 +127,12 @@ void advise(const char *advice, ...)\n \n int advice_enabled(enum advice_type type)\n {\n-\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n+\tint enabled;\n+\n+\tif (!git_env_bool(GIT_ADVICE, 1))\n+\t\treturn 0;\n+\n+\tenabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n \n \tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n \t\treturn enabled &&\ndiff --git a/environment.h b/environment.h\nindex 05fd94d7be..26e87843e1 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -57,6 +57,13 @@ const char *getenv_safe(struct strvec *argv, const char *name);\n #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n #define GIT_ATTR_SOURCE_ENVIRONMENT \"GIT_ATTR_SOURCE\"\n \n+/*\n+ * Environment variable used to propagate the --no-advice global option to the\n+ * advice_enabled() helper, even when run in a subprocess.\n+ * This is an internal variable that should not be set by the user.\n+ */\n+#define GIT_ADVICE \"GIT_ADVICE\"\n+\n /*\n * Environment variable used in handshaking the wire protocol.\n * Contains a colon ':' separated list of keys with optional values\ndiff --git a/git.c b/git.c\nindex 654d615a18..6283d126e5 100644\n--- a/git.c\n+++ b/git.c\n@@ -38,7 +38,7 @@ const char git_usage_string[] =\n \t \" [--exec-path[=]] [--html-path] [--man-path] [--info-path]\\n\"\n \t \" [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\\n\"\n \t \" [--git-dir=] [--work-tree=] [--namespace=]\\n\"\n-\t \" [--config-env==] []\");\n+\t \" [--config-env==] [--no-advice] []\");\n \n const char git_more_info_string[] =\n \tN_(\"'git help -a' and 'git help -g' list available subcommands and some\\n\"\n@@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--no-advice\")) {\n+\t\t\tsetenv(GIT_ADVICE, \"0\", 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 0dcfb760a2..2ce2d4668a 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'advice should not be printed when --no-advice is used' '\n+\tcat << EOF > expect &&\n+On branch master\n+\n+No commits yet\n+\n+Untracked files:\n+\tREADME\n+\n+nothing added to commit but untracked files present\n+EOF\n+\n+\tgit init advice-test &&\n+ test_when_finished \"rm -fr advice-test\" &&\n+ cd advice-test &&\n+ touch README &&\n+ git --no-advice status > ../actual &&\n+ test_cmp ../expect ../actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"493730","messageId":"ZjB_OJUQt7lbkOgz@tanuki","threadId":"61356","inReplyTo":"20240430014724.83813-2-james@jamesliu.io","subject":"Re: [PATCH v3 1/1] advice: add --no-advice global option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-30T05:18:48Z","receivedAt":"2024-04-30T05:18:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 11:47:24AM +1000, James Liu wrote:\n[snip]\n> diff --git a/environment.h b/environment.h\n> index 05fd94d7be..26e87843e1 100644\n> --- a/environment.h\n> +++ b/environment.h\n> @@ -57,6 +57,13 @@ const char *getenv_safe(struct strvec *argv, const char *name);\n> #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n> #define GIT_ATTR_SOURCE_ENVIRONMENT \"GIT_ATTR_SOURCE\"\n> \n> +/*\n> + * Environment variable used to propagate the --no-advice global option to the\n> + * advice_enabled() helper, even when run in a subprocess.\n> + * This is an internal variable that should not be set by the user.\n> + */\n> +#define GIT_ADVICE \"GIT_ADVICE\"\n\nAlmost all of the other defines in this file have an \"_ENVIRONMENT\"\nsuffix. We should probably do the same here to mark it as the name of\nthe environment variable, which otherwise wouldn't be obvious.\n\n> /*\n> * Environment variable used in handshaking the wire protocol.\n> * Contains a colon ':' separated list of keys with optional values\n> diff --git a/git.c b/git.c\n> index 654d615a18..6283d126e5 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -38,7 +38,7 @@ const char git_usage_string[] =\n> \t \" [--exec-path[=]] [--html-path] [--man-path] [--info-path]\\n\"\n> \t \" [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\\n\"\n> \t \" [--git-dir=] [--work-tree=] [--namespace=]\\n\"\n> -\t \" [--config-env==] []\");\n> +\t \" [--config-env==] [--no-advice] []\");\n> \n> const char git_more_info_string[] =\n> \tN_(\"'git help -a' and 'git help -g' list available subcommands and some\\n\"\n> @@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\tsetenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);\n> \t\t\tif (envchanged)\n> \t\t\t\t*envchanged = 1;\n> +\t\t} else if (!strcmp(cmd, \"--no-advice\")) {\n> +\t\t\tsetenv(GIT_ADVICE, \"0\", 1);\n> +\t\t\tif (envchanged)\n> +\t\t\t\t*envchanged = 1;\n> \t\t} else {\n> \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n> \t\t\tusage(git_usage_string);\n> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> index 0dcfb760a2..2ce2d4668a 100755\n> --- a/t/t0018-advice.sh\n> +++ b/t/t0018-advice.sh\n> @@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'advice should not be printed when --no-advice is used' '\n> +\tcat << EOF > expect &&\n\nWe typically write those `cat >expect <<-EOF`. I assume you cannot use\nthe dash here because the \"README\" string is indented by a tab. But the\norder of redirects should still be reversed.\n\n> +On branch master\n> +\n> +No commits yet\n> +\n> +Untracked files:\n> +\tREADME\n> +\n> +nothing added to commit but untracked files present\n> +EOF\n> +\n> +\tgit init advice-test &&\n> + test_when_finished \"rm -fr advice-test\" &&\n\nLet's run `test_when_finished` before creating the repo.\n\n> + cd advice-test &&\n> + touch README &&\n> + git --no-advice status > ../actual &&\n> + test_cmp ../expect ../actual\n\nNit: these should be indented by tabs, not spaces.\n\n> +'\n\nWe could add two more tests that explicitly set the environment\nvariable, once to `true` and once to `false`. This would at least\nsomewhat represent the case of running Git subcommands, even though we\ncannot test whether Git actually knows to set the envvar like this.\n\nPatrick\n\n> test_done\n> -- \n> 2.44.0\n> \n> \n"},{"id":"493733","messageId":"8bbb043a6259d94312fdbcecb991da33@manjaro.org","threadId":"61356","inReplyTo":"ZjB_OJUQt7lbkOgz@tanuki","subject":"Re: [PATCH v3 1/1] advice: add --no-advice global option","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-30T06:24:33Z","receivedAt":"2024-04-30T06:24:35Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello James,\n\nOn 2024-04-30 07:18, Patrick Steinhardt wrote:\n> On Tue, Apr 30, 2024 at 11:47:24AM +1000, James Liu wrote:\n>> +/*\n>> + * Environment variable used to propagate the --no-advice global \n>> option to the\n>> + * advice_enabled() helper, even when run in a subprocess.\n>> + * This is an internal variable that should not be set by the user.\n>> + */\n>> +#define GIT_ADVICE \"GIT_ADVICE\"\n> \n> Almost all of the other defines in this file have an \"_ENVIRONMENT\"\n> suffix. We should probably do the same here to mark it as the name of\n> the environment variable, which otherwise wouldn't be obvious.\n\nJust for the record, I already pointed that out... [1]\n\n> We could add two more tests that explicitly set the environment\n> variable, once to `true` and once to `false`. This would at least\n> somewhat represent the case of running Git subcommands, even though we\n> cannot test whether Git actually knows to set the envvar like this.\n\n... and this as well. [1]\n\n[1] \nhttps://lore.kernel.org/git/37512328b1f3db4e8075bdb4beeb8929@manjaro.org/\n"},{"id":"493790","messageId":"xmqqbk5qstjt.fsf@gitster.g","threadId":"61356","inReplyTo":"20240430014724.83813-2-james@jamesliu.io","subject":"Re: [PATCH v3 1/1] advice: add --no-advice global option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-30T16:29:26Z","receivedAt":"2024-04-30T16:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Liu writes:\n\n> Advice hints must be disabled individually by setting the relevant\n> advice.* variables to false in the Git configuration. For server-side\n> and scripted usages of Git where hints aren't necessary, it can be\n> cumbersome to maintain configuration to ensure all advice hints are\n\nIt is not that scripted use decline hints because they are not\n\"necessary\"; it is they find the hints harmful for whatever reason.\n\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 7a1b112a3e..ef1d9dce5d 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -13,7 +13,7 @@ SYNOPSIS\n> [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n> [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n> [--git-dir=] [--work-tree=] [--namespace=]\n> - [--config-env==] []\n> + [--config-env==] [--no-advice] []\n\nMove the new one near [--no-replace-objects] to group the --no-*\noptions together.\n\nThis is not a fault of this patch, but I notice \"--no-lazy-fetch\"\nand \"--no-optional-locks\" are not listed here (there may be others,\nI didn't check too deeply). It might make sense to have a separate\nclean-up step to move the --no-* \"ordinarily we would never want to\ndisable these wholesale, but for very special needs here are the\nknobs to toggle them off\" options together in the DESCRIPTION\nsection and add missing ones to the SYNOPSIS section.\n\n> diff --git a/advice.c b/advice.c\n> index 75111191ad..abad9ccaa2 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -2,6 +2,7 @@\n> #include \"advice.h\"\n> #include \"config.h\"\n> #include \"color.h\"\n> +#include \"environment.h\"\n> #include \"gettext.h\"\n> #include \"help.h\"\n> #include \"string-list.h\"\n> @@ -126,7 +127,12 @@ void advise(const char *advice, ...)\n> \n> int advice_enabled(enum advice_type type)\n> {\n> -\tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n> +\tint enabled;\n> +\n> +\tif (!git_env_bool(GIT_ADVICE, 1))\n> +\t\treturn 0;\n\nHow expensive is it to check git_env_bool() every time without\ncaching the parsing of the value given to the environment variable?\nEverybody pays this price but for most users who do not set the\nenvironment variable it is not a price we want them to pay.\n\nOne way to solve that might be to just insert these\n\n\tstatic int advice_enabled = -1;\n\n\tif (advice_enabled < 0)\n\t\tadvice_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, 1)\n\tif (!advice_enabled)\n\t\treturn 0;\n\nand leave everything else intact. We do not even need to split the\ndeclalation+initialization into a ...\n\n> +\tenabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n\n... separate assignment.\n\n> +/*\n> + * Environment variable used to propagate the --no-advice global option to the\n> + * advice_enabled() helper, even when run in a subprocess.\n> + * This is an internal variable that should not be set by the user.\n> + */\n> +#define GIT_ADVICE \"GIT_ADVICE\"\n\nAs Patrick pointed out, this should be GIT_ADVICE_ENVIRONMENT or\nsomething.\n\n> diff --git a/git.c b/git.c\n> index 654d615a18..6283d126e5 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -38,7 +38,7 @@ const char git_usage_string[] =\n> \t \" [--exec-path[=]] [--html-path] [--man-path] [--info-path]\\n\"\n> \t \" [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\\n\"\n> \t \" [--git-dir=] [--work-tree=] [--namespace=]\\n\"\n> -\t \" [--config-env==] []\");\n> +\t \" [--config-env==] [--no-advice] []\");\n\nDitto.\n\n> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\n> index 0dcfb760a2..2ce2d4668a 100755\n> --- a/t/t0018-advice.sh\n> +++ b/t/t0018-advice.sh\n> @@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'advice should not be printed when --no-advice is used' '\n> +\tcat << EOF > expect &&\n\nStyle. Between the redirection operator and the file that the\noperator operates on, there should be no SP.\n\n> +On branch master\n> +\n> +No commits yet\n> +\n> +Untracked files:\n> +\tREADME\n\nSomething like\n\n\tq_to_tab >expect <<\\-EOF\n\tOn branch master\n\t...\n\tUntracked files:\n\tQREADME\n\t\n\tnothing added to ...\n\tEOF\n\nor\n\n\tsed -e \"s/^|//\" >expect <<\\-EOF\n\tOn branch master\n\t...\n\tUntracked files:\n\t|\tREADME\n\t\n\tnothing added to ...\n\tEOF\n\nwould work better.\n\n> +\tgit init advice-test &&\n> + test_when_finished \"rm -fr advice-test\" &&\n\nFunny indentation. Also if \"git init\" fails in the middle, after\ncreating the directory but before completing, your when-finished\nhandler fails to register.\n\n> + cd advice-test &&\n\nDo not chdir around without being inside a subshell. If we have to\nadd new tests later to this script, you do not want them to start in\nthe directory that you are going to remove at the end of this test.\n\n> + touch README &&\n\nWhen you do not care about the timestamp, avoid using \"touch\". Writing\n\n\t>README &&\n\ninstead would make it clear that you ONLY care about the presense of\nthe file, not even its contents.\n\n> + git --no-advice status > ../actual &&\n> + test_cmp ../expect ../actual\n\nSo, taken together:\n\n\t(\n\t\tcd advice-test &&\n\t\t>README &&\n\t\tgit --no-advice status\n\t) >actual &&\n\ttest_cmp expect actual\n\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"493967","messageId":"20240503071706.78109-1-james@jamesliu.io","threadId":"61356","inReplyTo":"20240430014724.83813-1-james@jamesliu.io","subject":"[PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-03T07:17:03Z","receivedAt":"2024-05-03T07:17:19Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Hi,\n\nThank you all for the comprehensive feedback. This is v4 of the patch\nseries to introduce a --no-advice option for silencing advice hints.\n\nThe main changes are:\n\n- Two preliminary commits to reorder/clean up the options documentation\n and usage string.\n- Caching the value of GIT_ADVICE.\n- Adding tests which explicitly set GIT_ADVICE to false and true.\n\nCheers,\nJames\n\nJames Liu (3):\n doc: clean up usage documentation for --no-* opts\n doc: add spacing around paginate options\n advice: add --no-advice global option\n\n Documentation/git.txt | 18 +++++++-----\n advice.c | 7 +++++\n environment.h | 7 +++++\n git.c | 11 +++++--\n t/t0018-advice.sh | 68 +++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 101 insertions(+), 10 deletions(-)\n\nRange-diff against v3:\n1: 55d5559586 < -: ---------- advice: add --no-advice global option\n-: ---------- > 1: ae3f45dadc doc: clean up usage documentation for --no-* opts\n-: ---------- > 2: 1b0019026a doc: add spacing around paginate options\n-: ---------- > 3: 31e73e6c0e advice: add --no-advice global option\n-- \n2.44.0\n\n"},{"id":"493968","messageId":"20240503071706.78109-2-james@jamesliu.io","threadId":"61356","inReplyTo":"20240503071706.78109-1-james@jamesliu.io","subject":"[PATCH v4 1/3] doc: clean up usage documentation for --no-* opts","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-03T07:17:04Z","receivedAt":"2024-05-03T07:17:23Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"We'll be adding another option to the --no-* class of options soon.\n\nClean up the existing options by grouping them together in the OPTIONS\nsection, and adding missing ones to the SYNOPSIS.\n\nSigned-off-by: James Liu \n---\n Documentation/git.txt | 14 +++++++-------\n git.c | 6 +++---\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 7a1b112a3e..7fa75350b2 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -11,9 +11,9 @@ SYNOPSIS\n [verse]\n 'git' [-v | --version] [-h | --help] [-C ] [-c =]\n [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n- [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n- [--git-dir=] [--work-tree=] [--namespace=]\n- [--config-env==] []\n+ [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]\n+ [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\n+ [--namespace=] [--config-env==] []\n \n DESCRIPTION\n -----------\n@@ -186,6 +186,10 @@ If you just want to run git as if it was started in `` then use\n \tThis is equivalent to setting the `GIT_NO_LAZY_FETCH`\n \tenvironment variable to `1`.\n \n+--no-optional-locks::\n+\tDo not perform optional operations that require locks. This is\n+\tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.\n+\n --literal-pathspecs::\n \tTreat pathspecs literally (i.e. no globbing, no pathspec magic).\n \tThis is equivalent to setting the `GIT_LITERAL_PATHSPECS` environment\n@@ -207,10 +211,6 @@ If you just want to run git as if it was started in `` then use\n \tAdd \"icase\" magic to all pathspec. This is equivalent to setting\n \tthe `GIT_ICASE_PATHSPECS` environment variable to `1`.\n \n---no-optional-locks::\n-\tDo not perform optional operations that require locks. This is\n-\tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.\n-\n --list-cmds=[,...]::\n \tList commands by group. This is an internal/experimental\n \toption and may change or be removed in the future. Supported\ndiff --git a/git.c b/git.c\nindex 654d615a18..7654571b75 100644\n--- a/git.c\n+++ b/git.c\n@@ -36,9 +36,9 @@ struct cmd_struct {\n const char git_usage_string[] =\n \tN_(\"git [-v | --version] [-h | --help] [-C ] [-c =]\\n\"\n \t \" [--exec-path[=]] [--html-path] [--man-path] [--info-path]\\n\"\n-\t \" [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\\n\"\n-\t \" [--git-dir=] [--work-tree=] [--namespace=]\\n\"\n-\t \" [--config-env==] []\");\n+\t \" [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\\n\"\n+\t \" [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\\n\"\n+\t \" [--namespace=] [--config-env==] []\");\n \n const char git_more_info_string[] =\n \tN_(\"'git help -a' and 'git help -g' list available subcommands and some\\n\"\n-- \n2.44.0\n\n"},{"id":"493969","messageId":"20240503071706.78109-3-james@jamesliu.io","threadId":"61356","inReplyTo":"20240503071706.78109-1-james@jamesliu.io","subject":"[PATCH v4 2/3] doc: add spacing around paginate options","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-03T07:17:05Z","receivedAt":"2024-05-03T07:17:25Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Make the documentation page consistent with the usage string printed by\ngit.c\n\nSigned-off-by: James Liu \n---\n Documentation/git.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 7fa75350b2..d11d3d0c86 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git' [-v | --version] [-h | --help] [-C ] [-c =]\n [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n- [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]\n+ [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\n [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\n [--namespace=] [--config-env==] []\n \n-- \n2.44.0\n\n"},{"id":"493970","messageId":"20240503071706.78109-4-james@jamesliu.io","threadId":"61356","inReplyTo":"20240503071706.78109-1-james@jamesliu.io","subject":"[PATCH v4 3/3] advice: add --no-advice global option","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-03T07:17:06Z","receivedAt":"2024-05-03T07:17:28Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"Advice hints must be disabled individually by setting the relevant\nadvice.* variables to false in the Git configuration. For server-side\nand scripted usages of Git where hints can be a hindrance, it can be\ncumbersome to maintain configuration to ensure all advice hints are\ndisabled in perpetuity. This is a particular concern in tests, where\nnew or changed hints can result in failed assertions.\n\nAdd a --no-advice global option to disable all advice hints from being\ndisplayed. This is independent of the toggles for individual advice\nhints. Use an internal environment variable (GIT_ADVICE) to ensure this\nconfiguration is propagated to the usage site, even if it executes in a\nsubprocess.\n\nSigned-off-by: James Liu \n---\n Documentation/git.txt | 8 +++--\n advice.c | 7 +++++\n environment.h | 7 +++++\n git.c | 9 ++++--\n t/t0018-advice.sh | 68 +++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 95 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex d11d3d0c86..a0c07f1db8 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -12,8 +12,9 @@ SYNOPSIS\n 'git' [-v | --version] [-h | --help] [-C ] [-c =]\n [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\n- [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\n- [--namespace=] [--config-env==] []\n+ [--no-optional-locks] [--no-advice] [--bare] [--git-dir=]\n+ [--work-tree=] [--namespace=] [--config-env==]\n+ []\n \n DESCRIPTION\n -----------\n@@ -190,6 +191,9 @@ If you just want to run git as if it was started in `` then use\n \tDo not perform optional operations that require locks. This is\n \tequivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.\n \n+--no-advice::\n+\tDisable all advice hints from being printed.\n+\n --literal-pathspecs::\n \tTreat pathspecs literally (i.e. no globbing, no pathspec magic).\n \tThis is equivalent to setting the `GIT_LITERAL_PATHSPECS` environment\ndiff --git a/advice.c b/advice.c\nindex 75111191ad..0a122c2020 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -2,6 +2,7 @@\n #include \"advice.h\"\n #include \"config.h\"\n #include \"color.h\"\n+#include \"environment.h\"\n #include \"gettext.h\"\n #include \"help.h\"\n #include \"string-list.h\"\n@@ -127,6 +128,12 @@ void advise(const char *advice, ...)\n int advice_enabled(enum advice_type type)\n {\n \tint enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;\n+\tstatic int globally_enabled = -1;\n+\n+\tif (globally_enabled < 0)\n+\t\tglobally_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, 1);\n+\tif (!globally_enabled)\n+\t\treturn 0;\n \n \tif (type == ADVICE_PUSH_UPDATE_REJECTED)\n \t\treturn enabled &&\ndiff --git a/environment.h b/environment.h\nindex 05fd94d7be..0b2d457f07 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -57,6 +57,13 @@ const char *getenv_safe(struct strvec *argv, const char *name);\n #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n #define GIT_ATTR_SOURCE_ENVIRONMENT \"GIT_ATTR_SOURCE\"\n \n+/*\n+ * Environment variable used to propagate the --no-advice global option to the\n+ * advice_enabled() helper, even when run in a subprocess.\n+ * This is an internal variable that should not be set by the user.\n+ */\n+#define GIT_ADVICE_ENVIRONMENT \"GIT_ADVICE\"\n+\n /*\n * Environment variable used in handshaking the wire protocol.\n * Contains a colon ':' separated list of keys with optional values\ndiff --git a/git.c b/git.c\nindex 7654571b75..637c61ca9c 100644\n--- a/git.c\n+++ b/git.c\n@@ -37,8 +37,9 @@ const char git_usage_string[] =\n \tN_(\"git [-v | --version] [-h | --help] [-C ] [-c =]\\n\"\n \t \" [--exec-path[=]] [--html-path] [--man-path] [--info-path]\\n\"\n \t \" [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\\n\"\n-\t \" [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\\n\"\n-\t \" [--namespace=] [--config-env==] []\");\n+\t \" [--no-optional-locks] [--no-advice] [--bare] [--git-dir=]\\n\"\n+\t \" [--work-tree=] [--namespace=] [--config-env==]\\n\"\n+\t \" []\");\n \n const char git_more_info_string[] =\n \tN_(\"'git help -a' and 'git help -g' list available subcommands and some\\n\"\n@@ -337,6 +338,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--no-advice\")) {\n+\t\t\tsetenv(GIT_ADVICE_ENVIRONMENT, \"0\", 1);\n+\t\t\tif (envchanged)\n+\t\t\t\t*envchanged = 1;\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex 0dcfb760a2..b02448ea16 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -29,4 +29,72 @@ test_expect_success 'advice should not be printed when config variable is set to\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'advice should not be printed when --no-advice is used' '\n+\tq_to_tab >expect <<-\\EOF &&\n+On branch master\n+\n+No commits yet\n+\n+Untracked files:\n+QREADME\n+\n+nothing added to commit but untracked files present\n+EOF\n+\n+\ttest_when_finished \"rm -fr advice-test\" &&\n+\tgit init advice-test &&\n+\t(\n+\t\tcd advice-test &&\n+\t\t>README &&\n+\t\tgit --no-advice status\n+\t) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'advice should not be printed when GIT_ADVICE is set to false' '\n+\tq_to_tab >expect <<-\\EOF &&\n+On branch master\n+\n+No commits yet\n+\n+Untracked files:\n+QREADME\n+\n+nothing added to commit but untracked files present\n+EOF\n+\n+\ttest_when_finished \"rm -fr advice-test\" &&\n+\tgit init advice-test &&\n+\t(\n+\t\tcd advice-test &&\n+\t\t>README &&\n+\t\tGIT_ADVICE=false git status\n+\t) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '\n+\tq_to_tab >expect <<-\\EOF &&\n+On branch master\n+\n+No commits yet\n+\n+Untracked files:\n+ (use \"git add ...\" to include in what will be committed)\n+QREADME\n+\n+nothing added to commit but untracked files present (use \"git add\" to track)\n+EOF\n+\n+\ttest_when_finished \"rm -fr advice-test\" &&\n+\tgit init advice-test &&\n+\t(\n+\t\tcd advice-test &&\n+\t\t>README &&\n+\t\tGIT_ADVICE=true git status\n+\t) >actual &&\n+\tcat actual > /tmp/actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.44.0\n\n"},{"id":"493971","messageId":"ab604f4b73eab1d9aae4e2a20ba0630e@manjaro.org","threadId":"61356","inReplyTo":"20240503071706.78109-1-james@jamesliu.io","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-03T07:31:04Z","receivedAt":"2024-05-03T07:31:12Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello James,\n\nOn 2024-05-03 09:17, James Liu wrote:\n> Documentation/git.txt | 18 +++++++-----\n> advice.c | 7 +++++\n> environment.h | 7 +++++\n> git.c | 11 +++++--\n> t/t0018-advice.sh | 68 +++++++++++++++++++++++++++++++++++++++++++\n> 5 files changed, 101 insertions(+), 10 deletions(-)\n> \n> Range-diff against v3:\n> 1: 55d5559586 < -: ---------- advice: add --no-advice global option\n> -: ---------- > 1: ae3f45dadc doc: clean up usage documentation for\n> --no-* opts\n> -: ---------- > 2: 1b0019026a doc: add spacing around paginate \n> options\n> -: ---------- > 3: 31e73e6c0e advice: add --no-advice global option\n\nJust a small suggestion... Perhaps the creation factor needs adjusting\nfor the range diff to actually be produced. To be clear, I don't mind\nthat it's missing here.\n"},{"id":"494002","messageId":"CAOLa=ZTTHhh0xVzhU7WPCOm7oE8s=hQRRLztVR3_=71euRcSLA@mail.gmail.com","threadId":"61356","inReplyTo":"20240503071706.78109-3-james@jamesliu.io","subject":"Re: [PATCH v4 2/3] doc: add spacing around paginate options","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-05-03T14:32:40Z","receivedAt":"2024-05-03T14:32:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"James Liu writes:\n\n> Make the documentation page consistent with the usage string printed by\n> git.c\n>\n\nNit: missing full-stop here.\n\n> Signed-off-by: James Liu \n> ---\n> Documentation/git.txt | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 7fa75350b2..d11d3d0c86 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> [verse]\n> 'git' [-v | --version] [-h | --help] [-C ] [-c =]\n> [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n> - [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]\n> + [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\n> [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\n> [--namespace=] [--config-env==] []\n>\n> --\n> 2.44.0\n"},{"id":"494003","messageId":"CAOLa=ZSHPVbd2pweNz+MtkdWgsZepsExODyd7UNx1774TzWV0A@mail.gmail.com","threadId":"61356","inReplyTo":"20240503071706.78109-1-james@jamesliu.io","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-05-03T14:35:55Z","receivedAt":"2024-05-03T14:35:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello James,\n\nJames Liu writes:\n\n> Hi,\n>\n> Thank you all for the comprehensive feedback. This is v4 of the patch\n> series to introduce a --no-advice option for silencing advice hints.\n>\n> The main changes are:\n>\n> - Two preliminary commits to reorder/clean up the options documentation\n> and usage string.\n> - Caching the value of GIT_ADVICE.\n> - Adding tests which explicitly set GIT_ADVICE to false and true.\n>\n> Cheers,\n> James\n>\n\nThis version looks good to me.\n\nThanks\nKarthik\n\n> James Liu (3):\n> doc: clean up usage documentation for --no-* opts\n> doc: add spacing around paginate options\n> advice: add --no-advice global option\n>\n> Documentation/git.txt | 18 +++++++-----\n> advice.c | 7 +++++\n> environment.h | 7 +++++\n> git.c | 11 +++++--\n> t/t0018-advice.sh | 68 +++++++++++++++++++++++++++++++++++++++++++\n> 5 files changed, 101 insertions(+), 10 deletions(-)\n>\n> Range-diff against v3:\n> 1: 55d5559586 < -: ---------- advice: add --no-advice global option\n> -: ---------- > 1: ae3f45dadc doc: clean up usage documentation for --no-* opts\n> -: ---------- > 2: 1b0019026a doc: add spacing around paginate options\n> -: ---------- > 3: 31e73e6c0e advice: add --no-advice global option\n> --\n> 2.44.0\n"},{"id":"494026","messageId":"xmqqedai959z.fsf@gitster.g","threadId":"61356","inReplyTo":"20240503071706.78109-1-james@jamesliu.io","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T17:25:28Z","receivedAt":"2024-05-03T17:25:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> Subject: Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints\n\nHuh? Do we have \"all\" option that disable everything?\n\n"},{"id":"494027","messageId":"xmqqa5l6950s.fsf@gitster.g","threadId":"61356","inReplyTo":"20240503071706.78109-2-james@jamesliu.io","subject":"Re: [PATCH v4 1/3] doc: clean up usage documentation for --no-* opts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T17:30:59Z","receivedAt":"2024-05-03T17:31:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Liu writes:\n\n> We'll be adding another option to the --no-* class of options soon.\n>\n> Clean up the existing options by grouping them together in the OPTIONS\n> section, and adding missing ones to the SYNOPSIS.\n\nNice. \n\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 7a1b112a3e..7fa75350b2 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -11,9 +11,9 @@ SYNOPSIS\n> [verse]\n> 'git' [-v | --version] [-h | --help] [-C ] [-c =]\n> [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n> - [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n> - [--git-dir=] [--work-tree=] [--namespace=]\n> - [--config-env==] []\n> + [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]\n> + [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\n> + [--namespace=] [--config-env==] []\n\nLooks sensible.\n\nThere still are a few options (like noglob-pathspecs) missing, but\ncleaning them up from this part of the documentation is totally\noutside the scope of this topic (#leftoverbits -- we either make\nthis exhaustive, or make it clear that this is not exhaustive).\n\nThanks.\n"},{"id":"494029","messageId":"xmqq5xvu94rq.fsf@gitster.g","threadId":"61356","inReplyTo":"CAOLa=ZTTHhh0xVzhU7WPCOm7oE8s=hQRRLztVR3_=71euRcSLA@mail.gmail.com","subject":"Re: [PATCH v4 2/3] doc: add spacing around paginate options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T17:36:25Z","receivedAt":"2024-05-03T17:36:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> James Liu writes:\n>\n>> Make the documentation page consistent with the usage string printed by\n>> git.c\n>>\n>\n> Nit: missing full-stop here.\n\nYes, but \"printed by git.c\" is an unsatisfying way to say this. We\ncan easily illustrate what the end user does to get it, for example:\n\n ... usage string given by \"git help git\".\n\nIt also makes it internally consistent with \"[-v | --version]\" that\nexists in the same document, which is even better reason to do this\nchange.\n\nThanks, both.\n\n>\n>> Signed-off-by: James Liu \n>> ---\n>> Documentation/git.txt | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/git.txt b/Documentation/git.txt\n>> index 7fa75350b2..d11d3d0c86 100644\n>> --- a/Documentation/git.txt\n>> +++ b/Documentation/git.txt\n>> @@ -11,7 +11,7 @@ SYNOPSIS\n>> [verse]\n>> 'git' [-v | --version] [-h | --help] [-C ] [-c =]\n>> [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n>> - [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]\n>> + [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\n>> [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\n>> [--namespace=] [--config-env==] []\n>>\n>> --\n>> 2.44.0\n"},{"id":"494035","messageId":"xmqqjzka7p2t.fsf_-_@gitster.g","threadId":"61356","inReplyTo":"ab604f4b73eab1d9aae4e2a20ba0630e@manjaro.org","subject":"Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T18:00:42Z","receivedAt":"2024-05-03T18:00:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Hello James,\n> ...\n>> Range-diff against v3:\n>> 1: 55d5559586 < -: ---------- advice: add --no-advice global option\n>> -: ---------- > 1: ae3f45dadc doc: clean up usage documentation for\n>> --no-* opts\n>> -: ---------- > 2: 1b0019026a doc: add spacing around paginate\n>> options\n>> -: ---------- > 3: 31e73e6c0e advice: add --no-advice global option\n>\n> Just a small suggestion... Perhaps the creation factor needs adjusting\n> for the range diff to actually be produced. To be clear, I don't mind\n> that it's missing here.\n\nI see this happen to too many series I see on the list. There are\ncases when the user knows that they are comparing an old and a new\niterations of the same series, e.g. running it from format-patch.\nWe probably should use a much higher creation factor than default to\nrun range-diff in such a context.\n\nIOW, this shouldn't have to be done by individual users, but by the\ntool.\n\nWhen I do a post-am check myself, I often use --creation-factor=999\nto force matching.\n\nPerhaps something along this line may not be a bad idea.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] format-patch: run range-diff with larger creation-factor\n\nWe see too often that a range-diff added to format-patch output\nshows too many \"unmatched\" patches. This is because the default\nvalue for creation-factor is set to a relatively low value.\n\nIt may be justified for other uses (like you have a yet-to-be-sent\nnew iteration of your series, and compare it against the 'seen'\nbranch that has an older iteration, probably with the '--left-only'\noption, to pick out only your patches while ignoring the others) of\n\"range-diff\" command, but when the command is run as part of the\nformat-patch, the user _knows_ and expects that the patches in the\nold and the new iterations roughly correspond to each other, so we\ncan and should use a much higher default.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/log.c | 2 +-\n range-diff.h | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/log.c w/builtin/log.c\nindex 4da7399905..7a019476c3 100644\n--- c/builtin/log.c\n+++ w/builtin/log.c\n@@ -2294,7 +2294,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (creation_factor < 0)\n-\t\tcreation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n+\t\tcreation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;\n \telse if (!rdiff_prev)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--creation-factor\", \"--range-diff\");\n \ndiff --git c/range-diff.h w/range-diff.h\nindex 04ffe217be..2f69f6a434 100644\n--- c/range-diff.h\n+++ w/range-diff.h\n@@ -6,6 +6,12 @@\n \n #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n \n+/*\n+ * A much higher value than the default, when we KNOW we are comparing\n+ * the same series (e.g., used when format-patch calls range-diff).\n+ */\n+#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999\n+\n struct range_diff_options {\n \tint creation_factor;\n \tunsigned dual_color:1;\n\n"},{"id":"494050","messageId":"CAPig+cRZEQjwhnMYvQ_0BjvXaR=3x=LQeJHzFOxC+ZPyOy9yJQ@mail.gmail.com","threadId":"61356","inReplyTo":"xmqqjzka7p2t.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-05-03T19:26:48Z","receivedAt":"2024-05-03T19:27:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 3, 2024 at 2:00 PM Junio C Hamano wrote:\n> Dragan Simic writes:\n> > Just a small suggestion... Perhaps the creation factor needs adjusting\n> > for the range diff to actually be produced. [...]\n>\n> I see this happen to too many series I see on the list. There are\n> cases when the user knows that they are comparing an old and a new\n> iterations of the same series, e.g. running it from format-patch.\n> We probably should use a much higher creation factor than default to\n> run range-diff in such a context.\n>\n> IOW, this shouldn't have to be done by individual users, but by the\n> tool.\n>\n> Perhaps something along this line may not be a bad idea.\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] format-patch: run range-diff with larger creation-factor\n>\n> We see too often that a range-diff added to format-patch output\n> shows too many \"unmatched\" patches. This is because the default\n> value for creation-factor is set to a relatively low value.\n>\n> It may be justified for other uses (like you have a yet-to-be-sent\n> new iteration of your series, and compare it against the 'seen'\n> branch that has an older iteration, probably with the '--left-only'\n> option, to pick out only your patches while ignoring the others) of\n> \"range-diff\" command, but when the command is run as part of the\n> format-patch, the user _knows_ and expects that the patches in the\n> old and the new iterations roughly correspond to each other, so we\n> can and should use a much higher default.\n>\n> Signed-off-by: Junio C Hamano \n\nÆvar had posted[1] pretty much the exact same patch a few years ago.\nAt the time, I had trouble understanding why `git range-diff` and `git\nformat-patch --range-dif` would need separate default creation\nfactors[2], and I still have trouble understanding why they should be\ndifferent. Aren't both commands addressing the same use-case of\ncomparing one version of a series against a subsequent version? In\nyour response[3], you seemed to agree with that observation and\nsuggested instead simply increasing the default creation factor for\nboth commands (which sounds reasonable to me).\n\n[1]: https://lore.kernel.org/git/87y35g9l18.fsf@evledraar.gmail.com/\n[2]: https://lore.kernel.org/git/CAPig+cRMiEcXVRYrgp+B3tcDreh41-a5_k0zABe+HNce0G=Cyw@mail.gmail.com/\n[3]: https://lore.kernel.org/git/xmqqzhps4uyq.fsf@gitster-ct.c.googlers.com/\n\n> ---\n> diff --git c/builtin/log.c w/builtin/log.c\n> index 4da7399905..7a019476c3 100644\n> --- c/builtin/log.c\n> +++ w/builtin/log.c\n> @@ -2294,7 +2294,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> if (creation_factor < 0)\n> - creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n> + creation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;\n> else if (!rdiff_prev)\n> die(_(\"the option '%s' requires '%s'\"), \"--creation-factor\", \"--range-diff\");\n> diff --git c/range-diff.h w/range-diff.h\n> index 04ffe217be..2f69f6a434 100644\n> --- c/range-diff.h\n> +++ w/range-diff.h\n> @@ -6,6 +6,12 @@\n> #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n>\n> +/*\n> + * A much higher value than the default, when we KNOW we are comparing\n> + * the same series (e.g., used when format-patch calls range-diff).\n> + */\n> +#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999\n"},{"id":"494057","messageId":"xmqqbk5m65i8.fsf@gitster.g","threadId":"61356","inReplyTo":"CAPig+cRZEQjwhnMYvQ_0BjvXaR=3x=LQeJHzFOxC+ZPyOy9yJQ@mail.gmail.com","subject":"Re: Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T19:48:47Z","receivedAt":"2024-05-03T19:48:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> different. Aren't both commands addressing the same use-case of\n> comparing one version of a series against a subsequent version? In\n> your response[3], you seemed to agree with that observation and\n> suggested instead simply increasing the default creation factor for\n> both commands (which sounds reasonable to me).\n\nI think Dscho's use case was compare only single updated series of\nhis with seen that has tons of irrelevant other patches, to have the\ncommand sift the patches belong to other topics away while making\ncomparison with the older incarnation of his topic. I never use the\ncommand for such a comparison, but I can imagine that in such a\ncontext lower criteria may help reduce false matches of his new\ncommits with unrelated commits on 'seen'.\n"},{"id":"494060","messageId":"xmqq4jbe64l9.fsf@gitster.g","threadId":"61356","inReplyTo":"xmqqbk5m65i8.fsf@gitster.g","subject":"Re: Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-03T20:08:34Z","receivedAt":"2024-05-03T20:08:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> different. Aren't both commands addressing the same use-case of\n>> comparing one version of a series against a subsequent version? In\n>> your response[3], you seemed to agree with that observation and\n>> suggested instead simply increasing the default creation factor for\n>> both commands (which sounds reasonable to me).\n>\n> I think Dscho's use case was compare only single updated series of\n> his with seen that has tons of irrelevant other patches, to have the\n> command sift the patches belong to other topics away while making\n> comparison with the older incarnation of his topic. I never use the\n> command for such a comparison, but I can imagine that in such a\n> context lower criteria may help reduce false matches of his new\n> commits with unrelated commits on 'seen'.\n\nIt seems that Dscho was in agreement that format-patch's use case\nshould try to be more aggressive at least back then. In the message\nin the thread you pointed\n\n https://lore.kernel.org/git/nycvar.QRO.7.76.6.1903211209280.41@tvgsbejvaqbjf.bet/\n\nhe does not give us the exact reason why he does not think the \"more\naggressive\" mode is not suitable for other use cases, though.\n\nA similar thread was raised more recently:\n\n https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/\n\nAlso, there was an attempt to clarify what the \"factor\" really\nmeant, but we did not get a real conclusion other than the UNIT to\nmeasure the \"factor\" is called \"percent\" (without specifying what\n100% is relative to, \"percent\" does not mean much to guide users\nto guess what the right value would be).\n\n https://lore.kernel.org/git/85snn12q-po05-osqs-n1o0-n6040392q01q@tzk.qr/\n\nSo, yes, --creation-factor= is messy, I think a very high\nvalue, much higher than the hardcoded default of 60, is more\nappropriate for use in format-patch, but we do not know what bad\neffect it would have if we used much higher default everywhere.\n\n\n"},{"id":"494065","messageId":"CAPig+cT-xk13jGEUmpT5QmwVYTxKNUZsu9xJ=Ph3h4AzKHg6pA@mail.gmail.com","threadId":"61356","inReplyTo":"xmqq4jbe64l9.fsf@gitster.g","subject":"Re: Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-05-03T21:24:05Z","receivedAt":"2024-05-03T21:24:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 3, 2024 at 4:08 PM Junio C Hamano wrote:\n> It seems that Dscho was in agreement that format-patch's use case\n> should try to be more aggressive at least back then. In the message\n> in the thread you pointed\n>\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.1903211209280.41@tvgsbejvaqbjf.bet/\n>\n> he does not give us the exact reason why he does not think the \"more\n> aggressive\" mode is not suitable for other use cases, though.\n\nHaving an answer to that question could be helpful.\n\n> A similar thread was raised more recently:\n>\n> https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/\n\nI think I missed this thread.\n\n> Also, there was an attempt to clarify what the \"factor\" really\n> meant, but we did not get a real conclusion other than the UNIT to\n> measure the \"factor\" is called \"percent\" (without specifying what\n> 100% is relative to, \"percent\" does not mean much to guide users\n> to guess what the right value would be).\n>\n> https://lore.kernel.org/git/85snn12q-po05-osqs-n1o0-n6040392q01q@tzk.qr/\n\nI recall this discussion, as well as its followup (in which I\nproclaimed my continuing lack of understanding of what creation-factor\nreally represents):\n\nhttps://lore.kernel.org/git/CAPig+cRp4N=6EktoisKAH09aVAPkPgZfHJYcB5pJFJ-CUpBHFA@mail.gmail.com/\n\n> So, yes, --creation-factor= is messy, I think a very high\n> value, much higher than the hardcoded default of 60, is more\n> appropriate for use in format-patch, but we do not know what bad\n> effect it would have if we used much higher default everywhere.\n\nAt this point in time, I'm not particularly against giving `git\nformat-patch` its own default creation-factor.\n\nMy only worry (and perhaps it's very minor) is that separate default\ncreation-factors for `git range-diff` and `git format-patch\n--range-diff` could catch the unwary off-guard when invoking `git\nrange-diff` to manually check the difference between an old an new\nversion of a series before finally invoking `git format-patch\n--range-diff` to actually create the patches for sending. (I've done\nthis myself on a very few occasions, but perhaps not often enough to\nwarrant the concern(?).)\n"},{"id":"494088","messageId":"34d77e4d-6edb-99d0-7fc5-fea5224654c7@gmx.de","threadId":"61356","inReplyTo":"CAPig+cT-xk13jGEUmpT5QmwVYTxKNUZsu9xJ=Ph3h4AzKHg6pA@mail.gmail.com","subject":"Re: Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-05-05T11:03:35Z","receivedAt":"2024-05-05T11:03:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 3 May 2024, Eric Sunshine wrote:\n\n> On Fri, May 3, 2024 at 4:08 PM Junio C Hamano wrote:\n> > It seems that Dscho was in agreement that format-patch's use case\n> > should try to be more aggressive at least back then. In the message\n> > in the thread you pointed\n> >\n> > https://lore.kernel.org/git/nycvar.QRO.7.76.6.1903211209280.41@tvgsbejvaqbjf.bet/\n> >\n> > he does not give us the exact reason why he does not think the \"more\n> > aggressive\" mode is not suitable for other use cases, though.\n>\n> Having an answer to that question could be helpful.\n\nSure.\n\nThe `creation factor` essentially tells Git how confident the caller is that the\ntwo provided commit ranges contain only matching pairs of patches, no\nmatter how different they look.\n\nWhen calling `git format-patch --range-diff`, we can be _quite_ certain.\nNot necessarily 100% (in which case the creation factor would need to be a\nlot higher than 100), as it does happen that contributors have to drop a\npatch or two, and add a patch or two, sometimes in the same iteration.\n\nWhen calling `git range-diff` in general, we can be less certain about\nthat. In fact, users like me often call `git range-diff` to _determine_\nwhether there are obvious matches (for example, to see whether commits in\nGitGitGadget PRs have corresponding commits in `seen`).\n\nThat difference in certainty is the entire reason why I contend that\n`range-diff` and `format-patch --range-diff` need different defaults for\nthe creation factor.\n\n> > A similar thread was raised more recently:\n> >\n> > https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/\n>\n> I think I missed this thread.\n\nHeh. I had forgotten about it.\n\nCiao,\nJohannes\n"},{"id":"494102","messageId":"D123EO5XLSAG.1DARNM0DT2Z74@jamesliu.io","threadId":"61356","inReplyTo":"CAOLa=ZSHPVbd2pweNz+MtkdWgsZepsExODyd7UNx1774TzWV0A@mail.gmail.com","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-05T23:17:21Z","receivedAt":"2024-05-05T23:17:27Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Sat May 4, 2024 at 12:35 AM AEST, Karthik Nayak wrote:\n> Hello James,\n>\n> James Liu writes:\n>\n> > Hi,\n> >\n> > Thank you all for the comprehensive feedback. This is v4 of the patch\n> > series to introduce a --no-advice option for silencing advice hints.\n> >\n> > The main changes are:\n> >\n> > - Two preliminary commits to reorder/clean up the options documentation\n> > and usage string.\n> > - Caching the value of GIT_ADVICE.\n> > - Adding tests which explicitly set GIT_ADVICE to false and true.\n> >\n> > Cheers,\n> > James\n> >\n>\n> This version looks good to me.\n>\n> Thanks\n> Karthik\n>\n\nThanks for the review Karthik!\n\nCheers,\nJames\n"},{"id":"494103","messageId":"D123HB68ZKO0.3C5RSYGQNO9L3@jamesliu.io","threadId":"61356","inReplyTo":"xmqqedai959z.fsf@gitster.g","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-05T23:20:48Z","receivedAt":"2024-05-05T23:20:52Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:\n> > Subject: Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints\n>\n> Huh? Do we have \"all\" option that disable everything?\n\nOops, I think I mistakenly copied the subject from the original cover\nletter. I don't suppose this warrants a v5, does it?\n\nCheers,\nJames\n"},{"id":"494104","messageId":"D125SZ21UPI0.166ZOVLTOBCRH@jamesliu.io","threadId":"61356","inReplyTo":"D123HB68ZKO0.3C5RSYGQNO9L3@jamesliu.io","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-06T01:10:04Z","receivedAt":"2024-05-06T01:10:10Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Mon May 6, 2024 at 9:20 AM AEST, James Liu wrote:\n> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:\n> > > Subject: Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints\n> >\n> > Huh? Do we have \"all\" option that disable everything?\n>\n> Oops, I think I mistakenly copied the subject from the original cover\n> letter. I don't suppose this warrants a v5, does it?\n>\n> Cheers,\n> James\n\nAhh, I didn't see Karthik's feedback. Will prepare a new version.\n\nCheers,\nJames\n"},{"id":"494106","messageId":"D126FGCB7EYO.37L0VVA4ZH43L@jamesliu.io","threadId":"61356","inReplyTo":"xmqqa5l6950s.fsf@gitster.g","subject":"Re: [PATCH v4 1/3] doc: clean up usage documentation for --no-* opts","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-06T01:39:25Z","receivedAt":"2024-05-06T01:39:30Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Sat May 4, 2024 at 3:30 AM AEST, Junio C Hamano wrote:\n> James Liu writes:\n>\n> > We'll be adding another option to the --no-* class of options soon.\n> >\n> > Clean up the existing options by grouping them together in the OPTIONS\n> > section, and adding missing ones to the SYNOPSIS.\n>\n> Nice. \n>\n> > diff --git a/Documentation/git.txt b/Documentation/git.txt\n> > index 7a1b112a3e..7fa75350b2 100644\n> > --- a/Documentation/git.txt\n> > +++ b/Documentation/git.txt\n> > @@ -11,9 +11,9 @@ SYNOPSIS\n> > [verse]\n> > 'git' [-v | --version] [-h | --help] [-C ] [-c =]\n> > [--exec-path[=]] [--html-path] [--man-path] [--info-path]\n> > - [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]\n> > - [--git-dir=] [--work-tree=] [--namespace=]\n> > - [--config-env==] []\n> > + [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]\n> > + [--no-optional-locks] [--bare] [--git-dir=] [--work-tree=]\n> > + [--namespace=] [--config-env==] []\n>\n> Looks sensible.\n>\n> There still are a few options (like noglob-pathspecs) missing, but\n> cleaning them up from this part of the documentation is totally\n> outside the scope of this topic (#leftoverbits -- we either make\n> this exhaustive, or make it clear that this is not exhaustive).\n>\n> Thanks.\n\nI did notice noglob-pathspecs, but I wasn't sure if it came under the\nsame purview as the `--no-*` options (where a dash exists after \"no\").\n\nCheers,\nJames\n"},{"id":"494163","messageId":"xmqqbk5i3ncw.fsf_-_@gitster.g","threadId":"61356","inReplyTo":"34d77e4d-6edb-99d0-7fc5-fea5224654c7@gmx.de","subject":"Re* Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-06T16:40:31Z","receivedAt":"2024-05-06T16:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> That difference in certainty is the entire reason why I contend that\n> `range-diff` and `format-patch --range-diff` need different defaults for\n> the creation factor.\n>\n>> > A similar thread was raised more recently:\n>> >\n>> > https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/\n>>\n>> I think I missed this thread.\n>\n> Heh. I had forgotten about it.\n\nSo what's the conclusion of this discussion (have we reached one\nyet)?\n\nto me it sounds like everybody is on board to raise the default used\nfor format-patch. In the old thread I said \"I use something\nunreasonably high like 9999\", but my \"unreasonably high\" number\nthese days is 999, which was used in the earlier weatherbaloon\npatch. The old thread ended with an academic \"we know different\ndefaults are appropriate, but what is the right number then?\" but\nanything higher (like 999 or even 9999) is better than the default\nfor range-diff which is 60, I would think.\n\nHere is the patch, this time with a bit of documentation stolen from\nthe old weather-balloon by Ævar.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] format-patch: run range-diff with larger creation-factor\n\nWe see too often that a range-diff added to format-patch output\nshows too many \"unmatched\" patches. This is because the default\nvalue for creation-factor is set to a relatively low value.\n\nIt may be justified for other uses (like you have a yet-to-be-sent\nnew iteration of your series, and compare it against the 'seen'\nbranch that has an older iteration, probably with the '--left-only'\noption, to pick out only your patches while ignoring the others) of\n\"range-diff\" command, but when the command is run as part of the\nformat-patch, the user _knows_ and expects that the patches in the\nold and the new iterations roughly correspond to each other, so we\ncan and should use a much higher default.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-format-patch.txt | 5 +++++\n builtin/log.c | 2 +-\n range-diff.h | 6 ++++++\n 3 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 728bb3821c..b72f87b114 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -346,6 +346,11 @@ material (this may change in the future).\n \tbetween the previous and current series of patches by adjusting the\n \tcreation/deletion cost fudge factor. See linkgit:git-range-diff[1])\n \tfor details.\n++\n+Defaults to 999 (the linkgit:git-range-diff[1] uses 60), as the use\n+case is to show comparison with an older iteration of the same\n+topic and the tool should find more correspondence between the two\n+sets of patches.\n \n --notes[=]::\n --no-notes::\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c0a8bb95e9..73608ffef9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2274,7 +2274,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (creation_factor < 0)\n-\t\tcreation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n+\t\tcreation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;\n \telse if (!rdiff_prev)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--creation-factor\", \"--range-diff\");\n \ndiff --git a/range-diff.h b/range-diff.h\nindex 04ffe217be..2f69f6a434 100644\n--- a/range-diff.h\n+++ b/range-diff.h\n@@ -6,6 +6,12 @@\n \n #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n \n+/*\n+ * A much higher value than the default, when we KNOW we are comparing\n+ * the same series (e.g., used when format-patch calls range-diff).\n+ */\n+#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999\n+\n struct range_diff_options {\n \tint creation_factor;\n \tunsigned dual_color:1;\n-- \n2.45.0-31-gd4cc1ec35f\n\n"},{"id":"494164","messageId":"xmqq7cg63nbr.fsf@gitster.g","threadId":"61356","inReplyTo":"D123HB68ZKO0.3C5RSYGQNO9L3@jamesliu.io","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-06T16:41:12Z","receivedAt":"2024-05-06T16:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"James Liu\" writes:\n\n> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:\n>> > Subject: Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints\n>>\n>> Huh? Do we have \"all\" option that disable everything?\n>\n> Oops, I think I mistakenly copied the subject from the original cover\n> letter. I don't suppose this warrants a v5, does it?\n\nNo, I was just teasing you ;-).\n\n"},{"id":"494165","messageId":"xmqq1q6e3n0i.fsf@gitster.g","threadId":"61356","inReplyTo":"D125SZ21UPI0.166ZOVLTOBCRH@jamesliu.io","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-06T16:47:57Z","receivedAt":"2024-05-06T16:48:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"James Liu\" writes:\n\n> On Mon May 6, 2024 at 9:20 AM AEST, James Liu wrote:\n>> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:\n>> > > Subject: Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints\n>> >\n>> > Huh? Do we have \"all\" option that disable everything?\n>>\n>> Oops, I think I mistakenly copied the subject from the original cover\n>> letter. I don't suppose this warrants a v5, does it?\n>>\n>> Cheers,\n>> James\n>\n> Ahh, I didn't see Karthik's feedback. Will prepare a new version.\n\nI count two messages from Karthik for v4, one of them being \"this\nround looks good\", to which you said \"thanks\".\n\nIf you mean the other comment about a non-sentence in the proposed\nlog message of [2/3], the copy I have in 'seen' says this:\n\n doc: add spacing around paginate options\n\n Make the documentation page consistent with the usage string printed by\n \"git help git\" and consistent with the description of \"[-v | --version]\"\n option.\n\n Signed-off-by: James Liu \n Signed-off-by: Junio C Hamano \n\nIf that looks good enough to both of you, then we can probably\ndeclare victory and mark the topic for 'next'.\n\nThanks.\n"},{"id":"494208","messageId":"D12XUEC6DL6O.POVU09JJFHO9@jamesliu.io","threadId":"61356","inReplyTo":"xmqq1q6e3n0i.fsf@gitster.g","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-05-06T23:08:27Z","receivedAt":"2024-05-06T23:08:33Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Tue May 7, 2024 at 2:47 AM AEST, Junio C Hamano wrote:\n> I count two messages from Karthik for v4, one of them being \"this\n> round looks good\", to which you said \"thanks\".\n>\n> If you mean the other comment about a non-sentence in the proposed\n> log message of [2/3], the copy I have in 'seen' says this:\n>\n> doc: add spacing around paginate options\n>\n> Make the documentation page consistent with the usage string printed by\n> \"git help git\" and consistent with the description of \"[-v | --version]\"\n> option.\n>\n> Signed-off-by: James Liu \n> Signed-off-by: Junio C Hamano \n>\n> If that looks good enough to both of you, then we can probably\n> declare victory and mark the topic for 'next'.\n>\n> Thanks.\n\nYes I'm happy with that. Thanks Junio!\n\nCheers,\nJames\n\n"},{"id":"494212","messageId":"8dca4ecbf85f36e4264d7587fde4b0d5@manjaro.org","threadId":"61356","inReplyTo":"CAPig+cT-xk13jGEUmpT5QmwVYTxKNUZsu9xJ=Ph3h4AzKHg6pA@mail.gmail.com","subject":"Re: Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-07T00:01:53Z","receivedAt":"2024-05-07T00:01:55Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Eric,\n\nOn 2024-05-03 23:24, Eric Sunshine wrote:\n> On Fri, May 3, 2024 at 4:08 PM Junio C Hamano \n> wrote:\n>> So, yes, --creation-factor= is messy, I think a very high\n>> value, much higher than the hardcoded default of 60, is more\n>> appropriate for use in format-patch, but we do not know what bad\n>> effect it would have if we used much higher default everywhere.\n> \n> At this point in time, I'm not particularly against giving `git\n> format-patch` its own default creation-factor.\n> \n> My only worry (and perhaps it's very minor) is that separate default\n> creation-factors for `git range-diff` and `git format-patch\n> --range-diff` could catch the unwary off-guard when invoking `git\n> range-diff` to manually check the difference between an old an new\n> version of a series before finally invoking `git format-patch\n> --range-diff` to actually create the patches for sending. (I've done\n> this myself on a very few occasions, but perhaps not often enough to\n> warrant the concern(?).)\n\nI agree that having two different default values for the creation\nfactor in \"git format-patch\" and \"git range-diff\" is a valid concern,\nbut I think that the resulting benefits, in form of patch submissions\nthat don't lack the range diff contents, outweigh this concern.\n"},{"id":"494213","messageId":"5665eef37193e8a6894f100a554bce4d@manjaro.org","threadId":"61356","inReplyTo":"xmqqbk5i3ncw.fsf_-_@gitster.g","subject":"Re: Re* Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-07T00:11:29Z","receivedAt":"2024-05-07T00:11:31Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Junio,\n\nOn 2024-05-06 18:40, Junio C Hamano wrote:\n> So what's the conclusion of this discussion (have we reached one\n> yet)?\n> \n> to me it sounds like everybody is on board to raise the default used\n> for format-patch. In the old thread I said \"I use something\n> unreasonably high like 9999\", but my \"unreasonably high\" number\n> these days is 999, which was used in the earlier weatherbaloon\n> patch. The old thread ended with an academic \"we know different\n> defaults are appropriate, but what is the right number then?\" but\n> anything higher (like 999 or even 9999) is better than the default\n> for range-diff which is 60, I would think.\n> \n> Here is the patch, this time with a bit of documentation stolen from\n> the old weather-balloon by Ævar.\n\nAfter going through all the linked discussions, I think that introducing\nthe new default value for the creation factor in \"git format-patch\" is\nthe way to go. However, I still have one remaining concern.\n\nSee, the creation factor is described in the documentation as some fudge\nfactor, specified as a percentage. Without going through the actual\nsource code in detail, a question that pops up in my mind is why do we\nneed to use 999 or 9999 as the new default value? Shouldn't 99 or 100\nbe good enough instead, if it's a percentage? I'd assume that the same\nquestion might be asked by other Git users.\n\nAdmittedly, I haven't produced numerous range diffs, but for what I did,\n90 worked very well as the creation factor value. I even created some\naliases that specify 90 as the creation factor value.\n\n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] format-patch: run range-diff with larger \n> creation-factor\n> \n> We see too often that a range-diff added to format-patch output\n> shows too many \"unmatched\" patches. This is because the default\n> value for creation-factor is set to a relatively low value.\n> \n> It may be justified for other uses (like you have a yet-to-be-sent\n> new iteration of your series, and compare it against the 'seen'\n> branch that has an older iteration, probably with the '--left-only'\n> option, to pick out only your patches while ignoring the others) of\n> \"range-diff\" command, but when the command is run as part of the\n> format-patch, the user _knows_ and expects that the patches in the\n> old and the new iterations roughly correspond to each other, so we\n> can and should use a much higher default.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/git-format-patch.txt | 5 +++++\n> builtin/log.c | 2 +-\n> range-diff.h | 6 ++++++\n> 3 files changed, 12 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-format-patch.txt\n> b/Documentation/git-format-patch.txt\n> index 728bb3821c..b72f87b114 100644\n> --- a/Documentation/git-format-patch.txt\n> +++ b/Documentation/git-format-patch.txt\n> @@ -346,6 +346,11 @@ material (this may change in the future).\n> \tbetween the previous and current series of patches by adjusting the\n> \tcreation/deletion cost fudge factor. See linkgit:git-range-diff[1])\n> \tfor details.\n> ++\n> +Defaults to 999 (the linkgit:git-range-diff[1] uses 60), as the use\n> +case is to show comparison with an older iteration of the same\n> +topic and the tool should find more correspondence between the two\n> +sets of patches.\n> \n> --notes[=]::\n> --no-notes::\n> diff --git a/builtin/log.c b/builtin/log.c\n> index c0a8bb95e9..73608ffef9 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -2274,7 +2274,7 @@ int cmd_format_patch(int argc, const char\n> **argv, const char *prefix)\n> \t}\n> \n> \tif (creation_factor < 0)\n> -\t\tcreation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;\n> +\t\tcreation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;\n> \telse if (!rdiff_prev)\n> \t\tdie(_(\"the option '%s' requires '%s'\"), \"--creation-factor\", \n> \"--range-diff\");\n> \n> diff --git a/range-diff.h b/range-diff.h\n> index 04ffe217be..2f69f6a434 100644\n> --- a/range-diff.h\n> +++ b/range-diff.h\n> @@ -6,6 +6,12 @@\n> \n> #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60\n> \n> +/*\n> + * A much higher value than the default, when we KNOW we are comparing\n> + * the same series (e.g., used when format-patch calls range-diff).\n> + */\n> +#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999\n> +\n> struct range_diff_options {\n> \tint creation_factor;\n> \tunsigned dual_color:1;\n"},{"id":"494214","messageId":"xmqqikzqwjyb.fsf@gitster.g","threadId":"61356","inReplyTo":"5665eef37193e8a6894f100a554bce4d@manjaro.org","subject":"Re: Re* Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-07T00:21:16Z","receivedAt":"2024-05-07T00:21:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> See, the creation factor is described in the documentation as some fudge\n> factor, specified as a percentage. Without going through the actual\n> source code in detail, a question that pops up in my mind is why do we\n> need to use 999 or 9999 as the new default value? Shouldn't 99 or 100\n> be good enough instead, if it's a percentage? I'd assume that the same\n> question might be asked by other Git users.\n\nIt is very much deliberate to choose a value beyond 100. Setting\nthe value to such a large value was designed to force somebody to\nask that question: \"what does the value mean?\" [*], and in a sense\nit already has achieved one half of its objective ;-).\n\nWe never had an satisfactory update to the range-diff documentation\nwhen we discussed this parameter in the past. It was stated that\nthe unit to express the value for creation-factor was called\n\"percent\", but it seems that nobody had a good explanation that can\nbe given to a layperson what that percent means (e.g., what is it\nrelative to? what it means to have that value at 100% as opposed to\nsay 50%?). Somebody should come up with an easy-to-understand\nexplanation suitable for users after reading and digesting the\noverly technical \"algorithm\" section of the manual, and forcing\nourselves to do a good job at it is the other half of its objective.\n\n\n[Footnote]\n\n * Having more than 100 as a value that is measured in \"percent\" is\n not unusual, by the way. You can zoom into a picture at\n magnification level of 400%, for example.\n\n\n"},{"id":"494221","messageId":"68570a52a2f316eca9444d82d36d7aaf@manjaro.org","threadId":"61356","inReplyTo":"xmqqikzqwjyb.fsf@gitster.g","subject":"Re: Re* Re* [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-07T04:45:17Z","receivedAt":"2024-05-07T04:45:19Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-05-07 02:21, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> See, the creation factor is described in the documentation as some \n>> fudge\n>> factor, specified as a percentage. Without going through the actual\n>> source code in detail, a question that pops up in my mind is why do we\n>> need to use 999 or 9999 as the new default value? Shouldn't 99 or 100\n>> be good enough instead, if it's a percentage? I'd assume that the \n>> same\n>> question might be asked by other Git users.\n> \n> It is very much deliberate to choose a value beyond 100. Setting\n> the value to such a large value was designed to force somebody to\n> ask that question: \"what does the value mean?\" [*], and in a sense\n> it already has achieved one half of its objective ;-).\n\nAh, I see, it's also meant to provoke users' curiosity a bit. :)\n\n> We never had an satisfactory update to the range-diff documentation\n> when we discussed this parameter in the past. It was stated that\n> the unit to express the value for creation-factor was called\n> \"percent\", but it seems that nobody had a good explanation that can\n> be given to a layperson what that percent means (e.g., what is it\n> relative to? what it means to have that value at 100% as opposed to\n> say 50%?). Somebody should come up with an easy-to-understand\n> explanation suitable for users after reading and digesting the\n> overly technical \"algorithm\" section of the manual, and forcing\n> ourselves to do a good job at it is the other half of its objective.\n\nYes, the whole percentage thing is a bit unclear. I'm going through\nthe ALGORITHM section of the git-range-diff(1) man page, which could\nalso use some wording and formatting improvements. I'll see to send\na patch that improves it a bit later.\n\n> [Footnote]\n> \n> * Having more than 100 as a value that is measured in \"percent\" is\n> not unusual, by the way. You can zoom into a picture at\n> magnification level of 400%, for example.\n\nOf course. For example, I use 120% as the default scaling in my web\nbrowser, to compensate for the rather high value of the screen DPI.\n"},{"id":"494252","messageId":"CAOLa=ZTwdLPYqeDh=HPf6tQqBfeTjQV6syE_sxtgWE_Cj48tvg@mail.gmail.com","threadId":"61356","inReplyTo":"D12XUEC6DL6O.POVU09JJFHO9@jamesliu.io","subject":"Re: [PATCH v4 0/3] advice: add \"all\" option to disable all hints","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-05-07T06:23:29Z","receivedAt":"2024-05-07T06:23:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello,\n\n\"James Liu\" writes:\n\n> On Tue May 7, 2024 at 2:47 AM AEST, Junio C Hamano wrote:\n>> I count two messages from Karthik for v4, one of them being \"this\n>> round looks good\", to which you said \"thanks\".\n>>\n>> If you mean the other comment about a non-sentence in the proposed\n>> log message of [2/3], the copy I have in 'seen' says this:\n>>\n>> doc: add spacing around paginate options\n>>\n>> Make the documentation page consistent with the usage string printed by\n>> \"git help git\" and consistent with the description of \"[-v | --version]\"\n>> option.\n>>\n>> Signed-off-by: James Liu \n>> Signed-off-by: Junio C Hamano \n>>\n>> If that looks good enough to both of you, then we can probably\n>> declare victory and mark the topic for 'next'.\n>>\n>> Thanks.\n>\n> Yes I'm happy with that. Thanks Junio!\n>\n> Cheers,\n> James\n\nLooks good to me too :)\n\nThanks\nKarthik\n"},{"id":"494293","messageId":"xmqqbk5hyw30.fsf@gitster.g","threadId":"61356","inReplyTo":"20240503071706.78109-4-james@jamesliu.io","subject":"[PATCH v4 4/3] t0018: two small fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-08T00:40:51Z","receivedAt":"2024-05-08T00:40:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even though the three tests that were recently added started their\nhere-doc with \"<<-\\EOF\", it did not take advantage of that and\ninstead wrote the here-doc payload abut to the left edge. Use a tabs\nto indent these lines.\n\nMore importantly, because these all hardcode the expected output,\nwhich contains the current branch name, they break the CI job that\nuses 'main' as the default branch name.\n\nUse\n\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=trunk\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n\nbetween the test_description line and \". ./test-lib.sh\" line to\nforce the initial branch name to 'trunk' and expect it to show in\nthe output.\n\nSigned-off-by: Junio C Hamano \n---\n\n * I should have noticed these before merging the series to 'next',\n so here is a fix-up patch on top of the three patches.\n\n t/t0018-advice.sh | 41 ++++++++++++++++++++++-------------------\n 1 file changed, 22 insertions(+), 19 deletions(-)\n\ndiff --git c/t/t0018-advice.sh w/t/t0018-advice.sh\nindex b02448ea16..29306b367c 100755\n--- c/t/t0018-advice.sh\n+++ w/t/t0018-advice.sh\n@@ -2,6 +2,9 @@\n \n test_description='Test advise_if_enabled functionality'\n \n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=trunk\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -31,15 +34,15 @@ test_expect_success 'advice should not be printed when config variable is set to\n \n test_expect_success 'advice should not be printed when --no-advice is used' '\n \tq_to_tab >expect <<-\\EOF &&\n-On branch master\n+\tOn branch trunk\n \n-No commits yet\n+\tNo commits yet\n \n-Untracked files:\n-QREADME\n+\tUntracked files:\n+\tQREADME\n \n-nothing added to commit but untracked files present\n-EOF\n+\tnothing added to commit but untracked files present\n+\tEOF\n \n \ttest_when_finished \"rm -fr advice-test\" &&\n \tgit init advice-test &&\n@@ -53,15 +56,15 @@ EOF\n \n test_expect_success 'advice should not be printed when GIT_ADVICE is set to false' '\n \tq_to_tab >expect <<-\\EOF &&\n-On branch master\n+\tOn branch trunk\n \n-No commits yet\n+\tNo commits yet\n \n-Untracked files:\n-QREADME\n+\tUntracked files:\n+\tQREADME\n \n-nothing added to commit but untracked files present\n-EOF\n+\tnothing added to commit but untracked files present\n+\tEOF\n \n \ttest_when_finished \"rm -fr advice-test\" &&\n \tgit init advice-test &&\n@@ -75,16 +78,16 @@ EOF\n \n test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '\n \tq_to_tab >expect <<-\\EOF &&\n-On branch master\n+\tOn branch trunk\n \n-No commits yet\n+\tNo commits yet\n \n-Untracked files:\n- (use \"git add ...\" to include in what will be committed)\n-QREADME\n+\tUntracked files:\n+\t (use \"git add ...\" to include in what will be committed)\n+\tQREADME\n \n-nothing added to commit but untracked files present (use \"git add\" to track)\n-EOF\n+\tnothing added to commit but untracked files present (use \"git add\" to track)\n+\tEOF\n \n \ttest_when_finished \"rm -fr advice-test\" &&\n \tgit init advice-test &&\n\n\n\n"}]}