{"thread":{"id":"54937","subject":"[PATCH] for-each-repo: do nothing on empty config","startedAt":"2021-01-05T14:43:35Z","lastAt":"2021-01-08T02:31:53Z","messageCount":15,"participants":["Derrick Stolee via GitGitGadget","Eric Sunshine","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"413458","messageId":"pull.834.git.1609857770445.gitgitgadget@gmail.com","threadId":"54937","inReplyTo":null,"subject":"[PATCH] for-each-repo: do nothing on empty config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-05T14:42:50Z","receivedAt":"2021-01-05T14:43:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\n'git for-each-repo --config=X' should return success without calling any\nsubcommands when the config key 'X' has no value. The current\nimplementation instead segfaults.\n\nA user could run into this issue if they used 'git maintenance start' to\ninitialize their cron schedule using 'git for-each-repo\n--config=maintenance.repo ...' but then using 'git maintenance\nunregister' to remove the config option. (Note: 'git maintenance stop'\nwould remove the config _and_ remove the cron schedule.)\n\nAdd a simple test to ensure this works.\n\nReported-by: Andreas Bühmann <dev@uuml.de>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    for-each-repo: do nothing on empty config\n    \n    Thanks, Andreas, for drawing my attention to this bug.\n    \n    [1] https://github.com/gitgitgadget/git/issues/833\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-834%2Fderrickstolee%2Ffor-each-repo-empty-config-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-834/derrickstolee/for-each-repo-empty-config-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/834\n\n builtin/for-each-repo.c  | 3 +++\n t/t0068-for-each-repo.sh | 4 ++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\nindex 5bba623ff12..9b9d2364f1a 100644\n--- a/builtin/for-each-repo.c\n+++ b/builtin/for-each-repo.c\n@@ -51,6 +51,9 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n \tvalues = repo_config_get_value_multi(the_repository,\n \t\t\t\t\t     config_key);\n \n+\tif (!values)\n+\t\treturn result;\n+\n \tfor (i = 0; !result && i < values->nr; i++)\n \t\tresult = run_command_on_repo(values->items[i].string, &args);\n \ndiff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh\nindex 136b4ec8392..adc1b81826a 100755\n--- a/t/t0068-for-each-repo.sh\n+++ b/t/t0068-for-each-repo.sh\n@@ -27,4 +27,8 @@ test_expect_success 'run based on configured value' '\n \tgrep again message\n '\n \n+test_expect_success 'do nothing on empty config' '\n+\tgit for-each-repo --config=bogus.config -- these args would fail\n+'\n+\n test_done\n\nbase-commit: 4950b2a2b5c4731e4c9d5b803739a6979b23fed6\n-- \ngitgitgadget\n"},{"id":"413473","messageId":"CAPig+cQ4B6s7RzGH=1QhSc_2kKy-Mbp9fyK4VoTntdAfCT4d9A@mail.gmail.com","threadId":"54937","inReplyTo":"pull.834.git.1609857770445.gitgitgadget@gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-05T17:55:04Z","receivedAt":"2021-01-05T17:55:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2021 at 9:44 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> 'git for-each-repo --config=X' should return success without calling any\n> subcommands when the config key 'X' has no value. The current\n> implementation instead segfaults.\n> [...]\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\n> @@ -51,6 +51,9 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n>         values = repo_config_get_value_multi(the_repository,\n>                                              config_key);\n> +       if (!values)\n> +               return result;\n\nProbably not worth a re-roll, but the above has higher cognitive load than:\n\n    if (!value)\n        return 0;\n\nwhich indicates clearly that the command succeeded, whereas `return\nresult` makes the reader scan all the code above the conditional to\nfigure out what values `result` could possibly hold.\n\n> diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh\n> @@ -27,4 +27,8 @@ test_expect_success 'run based on configured value' '\n> +test_expect_success 'do nothing on empty config' '\n> +       git for-each-repo --config=bogus.config -- these args would fail\n> +'\n\nThe `these args would fail` arguments add mystery to the test,\nprompting the reader to wonder what exactly is going on: \"Fail how?\",\n\"Is it supposed to fail?\". It might be less confusing to use something\nmore direct like `nonexistent` or `does not exist`.\n"},{"id":"413524","messageId":"db7bf751-7541-59b9-a3a0-ec07e28eb9de@gmail.com","threadId":"54937","inReplyTo":"CAPig+cQ4B6s7RzGH=1QhSc_2kKy-Mbp9fyK4VoTntdAfCT4d9A@mail.gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-06T02:20:22Z","receivedAt":"2021-01-06T02:21:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/5/2021 12:55 PM, Eric Sunshine wrote:\n> On Tue, Jan 5, 2021 at 9:44 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> 'git for-each-repo --config=X' should return success without calling any\n>> subcommands when the config key 'X' has no value. The current\n>> implementation instead segfaults.\n>> [...]\n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>> ---\n>> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\n>> @@ -51,6 +51,9 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n>>         values = repo_config_get_value_multi(the_repository,\n>>                                              config_key);\n>> +       if (!values)\n>> +               return result;\n> \n> Probably not worth a re-roll, but the above has higher cognitive load than:\n> \n>     if (!value)\n>         return 0;\n>\n> which indicates clearly that the command succeeded, whereas `return\n> result` makes the reader scan all the code above the conditional to\n> figure out what values `result` could possibly hold.\n\nTrue. Looking at this again, it might be better to just update the\nloop to be\n\n\tfor (i = 0; values && i < values->nr; i++) {\n\nwhich would run the loop zero times when there are zero results.\n \n>> diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh\n>> @@ -27,4 +27,8 @@ test_expect_success 'run based on configured value' '\n>> +test_expect_success 'do nothing on empty config' '\n>> +       git for-each-repo --config=bogus.config -- these args would fail\n>> +'\n> \n> The `these args would fail` arguments add mystery to the test,\n> prompting the reader to wonder what exactly is going on: \"Fail how?\",\n> \"Is it supposed to fail?\". It might be less confusing to use something\n> more direct like `nonexistent` or `does not exist`.\n\nI guess the point is that if we actually did try running a subcommand\non a repo (for instance, accidentally running it on the current repo)\nthen the command would fail when running \"git these args would fail\".\n\nTo me, \"nonexistent\" or \"does not exist\" doesn't adequately describe\nthis (hypothetical) situation.\n\nPerhaps \"fail-subcommand\" might be better?\n\n-Stolee\n\n"},{"id":"413527","messageId":"CAPig+cRGWzz5n_PZ=_OiHy2hkmeru3=fo2zX3_hnsEhnPq+giQ@mail.gmail.com","threadId":"54937","inReplyTo":"db7bf751-7541-59b9-a3a0-ec07e28eb9de@gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-06T04:20:14Z","receivedAt":"2021-01-06T04:21:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2021 at 9:20 PM Derrick Stolee <stolee@gmail.com> wrote:\n> On 1/5/2021 12:55 PM, Eric Sunshine wrote:\n> > On Tue, Jan 5, 2021 at 9:44 AM Derrick Stolee via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> > Probably not worth a re-roll, but the above has higher cognitive load than:\n> >\n> >     if (!value)\n> >         return 0;\n> >\n> > which indicates clearly that the command succeeded, whereas `return\n> > result` makes the reader scan all the code above the conditional to\n> > figure out what values `result` could possibly hold.\n>\n> True. Looking at this again, it might be better to just update the\n> loop to be\n>\n>         for (i = 0; values && i < values->nr; i++) {\n>\n> which would run the loop zero times when there are zero results.\n\nI find the explicit `return 0` easier to reason about since I can see\nimmediately that it handles a particular boundary condition, after\nwhich I no longer have to think about that condition as I continue\nreading the code.\n\nThat said, I don't think it matters greatly one way or the other\nwhether you use `return result`, `return 0`, or adjust the loop\ncondition. It might matter if more functionality is added later, but\nwe don't have to worry about it now, and it's not worth re-rolling\njust for this, or spending a lot of time discussing it.\n\n> >> +       git for-each-repo --config=bogus.config -- these args would fail\n> >\n> > The `these args would fail` arguments add mystery to the test,\n> > prompting the reader to wonder what exactly is going on: \"Fail how?\",\n> > \"Is it supposed to fail?\". It might be less confusing to use something\n> > more direct like `nonexistent` or `does not exist`.\n>\n> I guess the point is that if we actually did try running a subcommand\n> on a repo (for instance, accidentally running it on the current repo)\n> then the command would fail when running \"git these args would fail\".\n>\n> To me, \"nonexistent\" or \"does not exist\" doesn't adequately describe\n> this (hypothetical) situation.\n>\n> Perhaps \"fail-subcommand\" might be better?\n\nI had never even looked at git-for-each-repo before, so I took the\ntime to read the documentation and the source code before writing this\nreply. Now that I understand what is supposed to happen, I might be\ntempted to suggest `this-command-wont-be-run` as an argument, but\nthat's a mouthful. If you want to be really explicit that it should\nfail if a bug gets re-introduced which causes the argument to be run\neven though the config is empty, then perhaps use `false`:\n\n    git for-each-repo --config=bogus.config -- false\n\nBy the way, when reading the documentation, some questions came to mind.\n\nIs the behavior implemented by this patch desirable? That is, if the\nuser mistypes the name of the configuration variable, would it be\nbetter to let the user know about the problem by returning an error\ncode rather than succeeding silently? Or do you foresee people\nintentionally running the command against an empty config variable?\nThat might be legitimate in automation situations. If legitimate to\nhave an empty config, then would it be helpful to somehow distinguish\nbetween an empty config variable and a non-existent one (assuming we\ncan even do that)?\n\nIs the API of this command ideal? It feels odd to force the user to\nspecify required input via a command-line option rather than just as a\npositional argument. In other words, since the config variable name is\nmandatory, an alternative invocation format would be:\n\n    git for-each-repo <config-var> <cmd>\n\nOr do you foresee future enhancements expanding the ways in which the\nrepo list can be specified (such as from a file or stdin or directly\nvia the command-line), in which case the `--config` option makes\nsense.\n"},{"id":"413541","messageId":"xmqqa6tmtrr1.fsf@gitster.c.googlers.com","threadId":"54937","inReplyTo":"db7bf751-7541-59b9-a3a0-ec07e28eb9de@gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T08:05:22Z","receivedAt":"2021-01-06T08:06:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> Probably not worth a re-roll, but the above has higher cognitive load than:\n>> \n>>     if (!value)\n>>         return 0;\n>>\n>> which indicates clearly that the command succeeded, whereas `return\n>> result` makes the reader scan all the code above the conditional to\n>> figure out what values `result` could possibly hold.\n>\n> True. Looking at this again, it might be better to just update the\n> loop to be\n>\n> \tfor (i = 0; values && i < values->nr; i++) {\n>\n> which would run the loop zero times when there are zero results.\n\nIt is misleading, though.  It forces readers to first assume that\nthe loop body may update \"values\" from non-NULL to NULL in some\ncondition and hunt for such a code in vain.\n\nIf some clean-up starts to become needed after the loop, the \"if the\nvalue array is empty, immediately return 0\" may have to be rewritten\nto \"if empty, jump to the clean-up part after the loop\", but until\nthen, what Eric gave us would be the easiest to read, I would think.\n\n> To me, \"nonexistent\" or \"does not exist\" doesn't adequately describe\n> this (hypothetical) situation.\n>\n> Perhaps \"fail-subcommand\" might be better?\n\nI wonder if \"false\" or \"exit 1\" would fit the bill.  In any case, a\ncomment may help, perhaps?\n\n\ttest_expect_success 'do nothing and succeed on empty/missing config' '\n\t\t# if this runs even once, \"false\" ensures a failure\n\t\tgit for-each-repo --config=bogus.config -- false\n\t'\n"},{"id":"413565","messageId":"3be0a434-d29f-a7b5-fb46-bb1fc1649d27@gmail.com","threadId":"54937","inReplyTo":"xmqqa6tmtrr1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-06T11:41:50Z","receivedAt":"2021-01-06T11:42:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/6/2021 3:05 AM, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n> \n>>> Probably not worth a re-roll, but the above has higher cognitive load than:\n>>>\n>>>     if (!value)\n>>>         return 0;\n>>>\n>>> which indicates clearly that the command succeeded, whereas `return\n>>> result` makes the reader scan all the code above the conditional to\n>>> figure out what values `result` could possibly hold.\n>>\n>> True. Looking at this again, it might be better to just update the\n>> loop to be\n>>\n>> \tfor (i = 0; values && i < values->nr; i++) {\n>>\n>> which would run the loop zero times when there are zero results.\n> \n> It is misleading, though.  It forces readers to first assume that\n> the loop body may update \"values\" from non-NULL to NULL in some\n> condition and hunt for such a code in vain.\n> \n> If some clean-up starts to become needed after the loop, the \"if the\n> value array is empty, immediately return 0\" may have to be rewritten\n> to \"if empty, jump to the clean-up part after the loop\", but until\n> then, what Eric gave us would be the easiest to read, I would think.\n\nOk. That works for me.\n\n>> To me, \"nonexistent\" or \"does not exist\" doesn't adequately describe\n>> this (hypothetical) situation.\n>>\n>> Perhaps \"fail-subcommand\" might be better?\n> \n> I wonder if \"false\" or \"exit 1\" would fit the bill.  In any case, a\n> comment may help, perhaps?\n> \n> \ttest_expect_success 'do nothing and succeed on empty/missing config' '\n> \t\t# if this runs even once, \"false\" ensures a failure\n> \t\tgit for-each-repo --config=bogus.config -- false\n> \t'\n\nI can add a comment, but keep in mind that this example would run the\nsubcommand as \"git false\". This isn't intended as an arbitrary script\nrunner, but a \"please run the same Git command on a list of repos\".\n\nThanks,\n-Stolee\n\n"},{"id":"413571","messageId":"63e618bd-a60e-0692-6489-06b4b0504895@gmail.com","threadId":"54937","inReplyTo":"CAPig+cRGWzz5n_PZ=_OiHy2hkmeru3=fo2zX3_hnsEhnPq+giQ@mail.gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-06T11:54:16Z","receivedAt":"2021-01-06T11:55:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/5/2021 11:20 PM, Eric Sunshine wrote:\n> On Tue, Jan 5, 2021 at 9:20 PM Derrick Stolee <stolee@gmail.com> wrote:\n>> On 1/5/2021 12:55 PM, Eric Sunshine wrote:\n>>> On Tue, Jan 5, 2021 at 9:44 AM Derrick Stolee via GitGitGadget\n>>> <gitgitgadget@gmail.com> wrote:\n>>> Probably not worth a re-roll, but the above has higher cognitive load than:\n>>>\n>>>     if (!value)\n>>>         return 0;\n>>>\n>>> which indicates clearly that the command succeeded, whereas `return\n>>> result` makes the reader scan all the code above the conditional to\n>>> figure out what values `result` could possibly hold.\n>>\n>> True. Looking at this again, it might be better to just update the\n>> loop to be\n>>\n>>         for (i = 0; values && i < values->nr; i++) {\n>>\n>> which would run the loop zero times when there are zero results.\n> \n> I find the explicit `return 0` easier to reason about since I can see\n> immediately that it handles a particular boundary condition, after\n> which I no longer have to think about that condition as I continue\n> reading the code.\n> \n> That said, I don't think it matters greatly one way or the other\n> whether you use `return result`, `return 0`, or adjust the loop\n> condition. It might matter if more functionality is added later, but\n> we don't have to worry about it now, and it's not worth re-rolling\n> just for this, or spending a lot of time discussing it.\n\nJunio made a good point that 'values' doesn't change during the loop,\nso I shouldn't use it in the sentinel. I'll use your \"return 0;\"\n\n>>>> +       git for-each-repo --config=bogus.config -- these args would fail\n>>>\n>>> The `these args would fail` arguments add mystery to the test,\n>>> prompting the reader to wonder what exactly is going on: \"Fail how?\",\n>>> \"Is it supposed to fail?\". It might be less confusing to use something\n>>> more direct like `nonexistent` or `does not exist`.\n>>\n>> I guess the point is that if we actually did try running a subcommand\n>> on a repo (for instance, accidentally running it on the current repo)\n>> then the command would fail when running \"git these args would fail\".\n>>\n>> To me, \"nonexistent\" or \"does not exist\" doesn't adequately describe\n>> this (hypothetical) situation.\n>>\n>> Perhaps \"fail-subcommand\" might be better?\n> \n> I had never even looked at git-for-each-repo before, so I took the\n> time to read the documentation and the source code before writing this\n> reply. Now that I understand what is supposed to happen, I might be\n> tempted to suggest `this-command-wont-be-run` as an argument, but\n> that's a mouthful. If you want to be really explicit that it should\n> fail if a bug gets re-introduced which causes the argument to be run\n> even though the config is empty, then perhaps use `false`:\n> \n>     git for-each-repo --config=bogus.config -- false\n\nI'm just going to repeat that this would run \"git false\". It achieves\nthe same goal where we interpret this as a failure if the subcommand\nis run.\n\n> By the way, when reading the documentation, some questions came to mind.\n> \n> Is the behavior implemented by this patch desirable? That is, if the\n> user mistypes the name of the configuration variable, would it be\n> better to let the user know about the problem by returning an error\n> code rather than succeeding silently? Or do you foresee people\n> intentionally running the command against an empty config variable?\n> That might be legitimate in automation situations. If legitimate to\n> have an empty config, then would it be helpful to somehow distinguish\n> between an empty config variable and a non-existent one (assuming we\n> can even do that)?\n\nAs mentioned in the message, this is the situation the background\nmaintenance would see if a user used 'git maintenance unregister' on\nall of their repos or removed the 'maintenance.repo' config values\nfrom their global config. I think it would be better to not start\nfailing the background commands.\n\nEven outside of that context, we have no way to specify an \"existing\nbut empty\" multi-valued config value over a non-existing config\nvalue. I'd rather prefer the interpretation \"I succeeded in doing\nnothing\" instead of \"I think you made a mistake, because there's\nnothing to do.\"\n\nCould we meet in the middle and print a warning to stderr?\n\n\twarning(_(\"supplied config key '%s' has no values\"));\n\nThat would present a useful message to users running this command\nmanually (or constructing automation) without breaking scripts\nthat parse the output.\n\n> Is the API of this command ideal? It feels odd to force the user to\n> specify required input via a command-line option rather than just as a\n> positional argument. In other words, since the config variable name is\n> mandatory, an alternative invocation format would be:\n> \n>     git for-each-repo <config-var> <cmd>\n> \n> Or do you foresee future enhancements expanding the ways in which the\n> repo list can be specified (such as from a file or stdin or directly\n> via the command-line), in which case the `--config` option makes\n> sense.\n\nI believe some voice interest in specifying a list of repositories\nby providing a directory instead of a config value. That would\nrequire scanning the immediate subdirectories to see if they are Git\nrepos.\n\nA --stdin option would also be a good addition, if desired.\n\nSince this command was intended for automation, not end-users, it\nseemed that future-proofing the arguments in this way would be a good\nidea. We want to remain flexible for future additions without\nbreaking compatibility.\n\nThanks,\n-Stolee\n"},{"id":"413592","messageId":"CAPig+cRCpgrhypq-0n5skiHYVXC9RrLDgC9vyi2k83p=6JuYcw@mail.gmail.com","threadId":"54937","inReplyTo":"63e618bd-a60e-0692-6489-06b4b0504895@gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-06T18:18:05Z","receivedAt":"2021-01-06T18:19:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 6, 2021 at 6:54 AM Derrick Stolee <stolee@gmail.com> wrote:\n> On 1/5/2021 11:20 PM, Eric Sunshine wrote:\n> > Is the behavior implemented by this patch desirable? That is, if the\n> > user mistypes the name of the configuration variable, would it be\n> > better to let the user know about the problem by returning an error\n> > code rather than succeeding silently? Or do you foresee people\n> > intentionally running the command against an empty config variable?\n>\n> As mentioned in the message, this is the situation the background\n> maintenance would see if a user used 'git maintenance unregister' on\n> all of their repos or removed the 'maintenance.repo' config values\n> from their global config. I think it would be better to not start\n> failing the background commands.\n>\n> Even outside of that context, we have no way to specify an \"existing\n> but empty\" multi-valued config value over a non-existing config\n> value. I'd rather prefer the interpretation \"I succeeded in doing\n> nothing\" instead of \"I think you made a mistake, because there's\n> nothing to do.\"\n>\n> Could we meet in the middle and print a warning to stderr?\n>\n>         warning(_(\"supplied config key '%s' has no values\"));\n>\n> That would present a useful message to users running this command\n> manually (or constructing automation) without breaking scripts\n> that parse the output.\n\nI don't know. My knee-jerk response is that such a warning could get\nannoying quickly if this is used mostly in an automated environment,\nand an empty config value is likely to come up in practice. In that\ncase, I'm somewhat negative on adding the warning. (I could perhaps\nsee a --dry-run or --verbose mode issuing the above warning, but\nthat's outside the scope of this series.)\n\n> > Is the API of this command ideal? It feels odd to force the user to\n> > specify required input via a command-line option rather than just as a\n> > positional argument. In other words, since the config variable name is\n> > mandatory, an alternative invocation format would be:\n> >\n> >     git for-each-repo <config-var> <cmd>\n> >\n> > Or do you foresee future enhancements expanding the ways in which the\n> > repo list can be specified (such as from a file or stdin or directly\n> > via the command-line), in which case the `--config` option makes\n> > sense.\n>\n> I believe some voice interest in specifying a list of repositories\n> by providing a directory instead of a config value. That would\n> require scanning the immediate subdirectories to see if they are Git\n> repos.\n>\n> A --stdin option would also be a good addition, if desired.\n>\n> Since this command was intended for automation, not end-users, it\n> seemed that future-proofing the arguments in this way would be a good\n> idea. We want to remain flexible for future additions without\n> breaking compatibility.\n\nFair enough.\n"},{"id":"413594","messageId":"pull.834.v2.git.1609960783988.gitgitgadget@gmail.com","threadId":"54937","inReplyTo":"pull.834.git.1609857770445.gitgitgadget@gmail.com","subject":"[PATCH v2] for-each-repo: do nothing on empty config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-06T19:19:43Z","receivedAt":"2021-01-06T19:20:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\n'git for-each-repo --config=X' should return success without calling any\nsubcommands when the config key 'X' has no value. The current\nimplementation instead segfaults.\n\nA user could run into this issue if they used 'git maintenance start' to\ninitialize their cron schedule using 'git for-each-repo\n--config=maintenance.repo ...' but then using 'git maintenance\nunregister' to remove the config option. (Note: 'git maintenance stop'\nwould remove the config _and_ remove the cron schedule.)\n\nAdd a simple test to ensure this works.\n\nReported-by: Andreas Bühmann <dev@uuml.de>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    for-each-repo: do nothing on empty config\n    \n    Thanks, Andreas, for drawing my attention to this bug.\n    \n    [1] https://github.com/gitgitgadget/git/issues/833\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-834%2Fderrickstolee%2Ffor-each-repo-empty-config-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-834/derrickstolee/for-each-repo-empty-config-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/834\n\nRange-diff vs v1:\n\n 1:  36dccbb8c20 ! 1:  a1f1300bacb for-each-repo: do nothing on empty config\n     @@ Commit message\n          Add a simple test to ensure this works.\n      \n          Reported-by: Andreas Bühmann <dev@uuml.de>\n     +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n      \n       ## builtin/for-each-repo.c ##\n     @@ builtin/for-each-repo.c: int cmd_for_each_repo(int argc, const char **argv, cons\n       \tvalues = repo_config_get_value_multi(the_repository,\n       \t\t\t\t\t     config_key);\n       \n     ++\t/*\n     ++\t * Do nothing on an empty list, which is equivalent to the case\n     ++\t * where the config variable does not exist at all.\n     ++\t */\n      +\tif (!values)\n     -+\t\treturn result;\n     ++\t\treturn 0;\n      +\n       \tfor (i = 0; !result && i < values->nr; i++)\n       \t\tresult = run_command_on_repo(values->items[i].string, &args);\n     @@ t/t0068-for-each-repo.sh: test_expect_success 'run based on configured value' '\n       '\n       \n      +test_expect_success 'do nothing on empty config' '\n     -+\tgit for-each-repo --config=bogus.config -- these args would fail\n     ++\t# will fail if any subcommand is run\n     ++\tgit for-each-repo --config=bogus.config -- false\n      +'\n      +\n       test_done\n\n\n builtin/for-each-repo.c  | 7 +++++++\n t/t0068-for-each-repo.sh | 5 +++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\nindex 5bba623ff12..52be64a4373 100644\n--- a/builtin/for-each-repo.c\n+++ b/builtin/for-each-repo.c\n@@ -51,6 +51,13 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n \tvalues = repo_config_get_value_multi(the_repository,\n \t\t\t\t\t     config_key);\n \n+\t/*\n+\t * Do nothing on an empty list, which is equivalent to the case\n+\t * where the config variable does not exist at all.\n+\t */\n+\tif (!values)\n+\t\treturn 0;\n+\n \tfor (i = 0; !result && i < values->nr; i++)\n \t\tresult = run_command_on_repo(values->items[i].string, &args);\n \ndiff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh\nindex 136b4ec8392..c3efba4da0b 100755\n--- a/t/t0068-for-each-repo.sh\n+++ b/t/t0068-for-each-repo.sh\n@@ -27,4 +27,9 @@ test_expect_success 'run based on configured value' '\n \tgrep again message\n '\n \n+test_expect_success 'do nothing on empty config' '\n+\t# will fail if any subcommand is run\n+\tgit for-each-repo --config=bogus.config -- false\n+'\n+\n test_done\n\nbase-commit: 4950b2a2b5c4731e4c9d5b803739a6979b23fed6\n-- \ngitgitgadget\n"},{"id":"413601","messageId":"xmqq1rexssqp.fsf@gitster.c.googlers.com","threadId":"54937","inReplyTo":"3be0a434-d29f-a7b5-fb46-bb1fc1649d27@gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T20:41:34Z","receivedAt":"2021-01-06T20:42:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> I wonder if \"false\" or \"exit 1\" would fit the bill.  In any case, a\n>> comment may help, perhaps?\n>> \n>> \ttest_expect_success 'do nothing and succeed on empty/missing config' '\n>> \t\t# if this runs even once, \"false\" ensures a failure\n>> \t\tgit for-each-repo --config=bogus.config -- false\n>> \t'\n>\n> I can add a comment, but keep in mind that this example would run the\n> subcommand as \"git false\". This isn't intended as an arbitrary script\n> runner, but a \"please run the same Git command on a list of repos\".\n\nAh, that is a good point.\n\nThe comment needs to explain:\n\n\t# the command fails if it attempts to run even once because\n\t# 'git false' does not exist\n\nand at that point, it does not have to be spelled 'false'.  It could\nbe 'no-such-git-subcommand' (and I wonder if that makes the comment\nunnecessary).\n\nThat reminds me.  If I have ~/bin/git-false and ~/bin on my $PATH,\nwould this test fail to catch breakage?\n\n\n"},{"id":"413602","messageId":"xmqqturtrdre.fsf@gitster.c.googlers.com","threadId":"54937","inReplyTo":"CAPig+cRGWzz5n_PZ=_OiHy2hkmeru3=fo2zX3_hnsEhnPq+giQ@mail.gmail.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T20:50:29Z","receivedAt":"2021-01-06T20:51:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Is the behavior implemented by this patch desirable? That is, if the\n> user mistypes the name of the configuration variable, would it be\n> better to let the user know about the problem by returning an error\n> code rather than succeeding silently? Or do you foresee people\n> intentionally running the command against an empty config variable?\n> That might be legitimate in automation situations. If legitimate to\n> have an empty config, then would it be helpful to somehow distinguish\n> between an empty config variable and a non-existent one (assuming we\n> can even do that)?\n\nI am guessing, without reading the mind of those who designed the\nfeature, that the envisioned use pattern is that a configuration\nvariable is designated, \"git for-each-ref --config=<var> <cmd>\" is\ncarved into stone in a script that is run periodically with the\nhardcoded variable name in it, but the value of the variable may\nchange from time to time.  If it is expected that the variable can\nsometimes be empty, then erroring out or even issuing a warning\nwould be counter-productive.\n\n> Is the API of this command ideal? It feels odd to force the user to\n> specify required input via a command-line option rather than just as a\n> positional argument. In other words, since the config variable name is\n> mandatory, an alternative invocation format would be:\n>\n>     git for-each-repo <config-var> <cmd>\n\nOr not to use any configuration variable (or not to have the\nfor-each-repo subcommand at all) and let the users write\nsomething along the lines of...\n\n\tgit config --get-all <config-var> |\n\twhile read repo\n\tdo\n\t\t( cd \"$repo\" && <cmd> )\n\tdone\n\nwhich is not all that bad and much more flexible.\n\n> Or do you foresee future enhancements expanding the ways in which the\n> repo list can be specified (such as from a file or stdin or directly\n> via the command-line), in which case the `--config` option makes\n> sense.\n\n"},{"id":"413609","messageId":"xmqqv9c9pwv0.fsf@gitster.c.googlers.com","threadId":"54937","inReplyTo":"xmqq1rexssqp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-06T21:40:51Z","receivedAt":"2021-01-06T21:42:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Derrick Stolee <stolee@gmail.com> writes:\n>\n>>> I wonder if \"false\" or \"exit 1\" would fit the bill.  In any case, a\n>>> comment may help, perhaps?\n>>> \n>>> \ttest_expect_success 'do nothing and succeed on empty/missing config' '\n>>> \t\t# if this runs even once, \"false\" ensures a failure\n>>> \t\tgit for-each-repo --config=bogus.config -- false\n>>> \t'\n>>\n>> I can add a comment, but keep in mind that this example would run the\n>> subcommand as \"git false\". This isn't intended as an arbitrary script\n>> runner, but a \"please run the same Git command on a list of repos\".\n>\n> Ah, that is a good point.\n>\n> The comment needs to explain:\n>\n> \t# the command fails if it attempts to run even once because\n> \t# 'git false' does not exist\n>\n> and at that point, it does not have to be spelled 'false'.  It could\n> be 'no-such-git-subcommand' (and I wonder if that makes the comment\n> unnecessary).\n>\n> That reminds me.  If I have ~/bin/git-false and ~/bin on my $PATH,\n> would this test fail to catch breakage?\n\nYes, I think $PATH in the test environment starts from the original\n$PATH and modified only by prepending, so my ~/bin/git-false would\nkick in.  We cannot reliably depend on the absence of a subcommand.\n\nWe can instead use\n\n\t# the whole thing would fail if for-each-ref iterated even\n\t# once, because 'git help --no-such-option' would fail\n\tgit for-each-ref --config=<var> -- help --no-such-option\n\nand I think that would be much more reliable; if an invocation of\n\"git help\" inside our test suite fails to refer to the \"git help\"\nfrom the version of Git being tested, we already have a serious\nproblem.\n\nThanks.\n"},{"id":"413633","messageId":"441209e0-2102-b055-867a-1b4747499181@gmail.com","threadId":"54937","inReplyTo":"xmqqv9c9pwv0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-07T02:00:51Z","receivedAt":"2021-01-07T02:01:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/6/2021 4:40 PM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Derrick Stolee <stolee@gmail.com> writes:\n>>\n>>>> I wonder if \"false\" or \"exit 1\" would fit the bill.  In any case, a\n>>>> comment may help, perhaps?\n>>>>\n>>>> \ttest_expect_success 'do nothing and succeed on empty/missing config' '\n>>>> \t\t# if this runs even once, \"false\" ensures a failure\n>>>> \t\tgit for-each-repo --config=bogus.config -- false\n>>>> \t'\n>>>\n>>> I can add a comment, but keep in mind that this example would run the\n>>> subcommand as \"git false\". This isn't intended as an arbitrary script\n>>> runner, but a \"please run the same Git command on a list of repos\".\n>>\n>> Ah, that is a good point.\n>>\n>> The comment needs to explain:\n>>\n>> \t# the command fails if it attempts to run even once because\n>> \t# 'git false' does not exist\n>>\n>> and at that point, it does not have to be spelled 'false'.  It could\n>> be 'no-such-git-subcommand' (and I wonder if that makes the comment\n>> unnecessary).\n>>\n>> That reminds me.  If I have ~/bin/git-false and ~/bin on my $PATH,\n>> would this test fail to catch breakage?\n> \n> Yes, I think $PATH in the test environment starts from the original\n> $PATH and modified only by prepending, so my ~/bin/git-false would\n> kick in.  We cannot reliably depend on the absence of a subcommand.\n> \n> We can instead use\n> \n> \t# the whole thing would fail if for-each-ref iterated even\n> \t# once, because 'git help --no-such-option' would fail\n> \tgit for-each-ref --config=<var> -- help --no-such-option\n> \n> and I think that would be much more reliable; if an invocation of\n> \"git help\" inside our test suite fails to refer to the \"git help\"\n> from the version of Git being tested, we already have a serious\n> problem.\n\nA very good point. I'll include this in v3.\n\nThanks,\n-Stolee\n"},{"id":"413642","messageId":"CAPig+cQ5_J4MU9CkG9kyjOTrYPFikkX4vG6AFQhczUT857H7pg@mail.gmail.com","threadId":"54937","inReplyTo":"xmqqturtrdre.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] for-each-repo: do nothing on empty config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-07T04:29:43Z","receivedAt":"2021-01-07T04:37:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 6, 2021 at 3:50 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > Is the API of this command ideal? It feels odd to force the user to\n> > specify required input via a command-line option rather than just as a\n> > positional argument. In other words, since the config variable name is\n> > mandatory, an alternative invocation format would be:\n> >\n> >     git for-each-repo <config-var> <cmd>\n>\n> Or not to use any configuration variable (or not to have the\n> for-each-repo subcommand at all) and let the users write\n> something along the lines of...\n>\n>         git config --get-all <config-var> |\n>         while read repo\n>         do\n>                 ( cd \"$repo\" && <cmd> )\n>         done\n>\n> which is not all that bad and much more flexible.\n\nI had the same thought/question about why git-for-each-repo exists,\nthough I didn't verbalize it since I assumed the reason was covered\nduring the original discussion or patch submission, which I did not\nfollow. I can see this command possibly being useful for Windows users\nwho don't necessarily have a Unix-like shell or MS PowerShell with\nwhich to open-code the loop you illustrated. This may be especially\nimportant when this is used for some sort of scheduled maintenance on\nWindows, as a guess.\n"},{"id":"413778","messageId":"pull.834.v3.git.1610073047135.gitgitgadget@gmail.com","threadId":"54937","inReplyTo":"pull.834.v2.git.1609960783988.gitgitgadget@gmail.com","subject":"[PATCH v3] for-each-repo: do nothing on empty config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-08T02:30:46Z","receivedAt":"2021-01-08T02:31:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\n'git for-each-repo --config=X' should return success without calling any\nsubcommands when the config key 'X' has no value. The current\nimplementation instead segfaults.\n\nA user could run into this issue if they used 'git maintenance start' to\ninitialize their cron schedule using 'git for-each-repo\n--config=maintenance.repo ...' but then using 'git maintenance\nunregister' to remove the config option. (Note: 'git maintenance stop'\nwould remove the config _and_ remove the cron schedule.)\n\nAdd a simple test to ensure this works. Use 'git help --no-such-option'\nas the potential subcommand to ensure that we will hit a failure if the\nsubcommand is ever run.\n\nReported-by: Andreas Bühmann <dev@uuml.de>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    for-each-repo: do nothing on empty config\n    \n    Thanks, Andreas, for drawing my attention to this bug.\n    \n    V3: update comment in test and use git help --no-such-option as the\n    subcommand to fail.\n    \n    [1] https://github.com/gitgitgadget/git/issues/833\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-834%2Fderrickstolee%2Ffor-each-repo-empty-config-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-834/derrickstolee/for-each-repo-empty-config-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/834\n\nRange-diff vs v2:\n\n 1:  a1f1300bacb ! 1:  31350c7845c for-each-repo: do nothing on empty config\n     @@ Commit message\n          unregister' to remove the config option. (Note: 'git maintenance stop'\n          would remove the config _and_ remove the cron schedule.)\n      \n     -    Add a simple test to ensure this works.\n     +    Add a simple test to ensure this works. Use 'git help --no-such-option'\n     +    as the potential subcommand to ensure that we will hit a failure if the\n     +    subcommand is ever run.\n      \n          Reported-by: Andreas Bühmann <dev@uuml.de>\n          Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n     +    Helped-by: Junio C Hamano <gitster@pobox.com>\n          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n      \n       ## builtin/for-each-repo.c ##\n     @@ t/t0068-for-each-repo.sh: test_expect_success 'run based on configured value' '\n       '\n       \n      +test_expect_success 'do nothing on empty config' '\n     -+\t# will fail if any subcommand is run\n     -+\tgit for-each-repo --config=bogus.config -- false\n     ++\t# the whole thing would fail if for-each-ref iterated even\n     ++\t# once, because \"git help --no-such-option\" would fail\n     ++\tgit for-each-repo --config=bogus.config -- help --no-such-option\n      +'\n      +\n       test_done\n\n\n builtin/for-each-repo.c  | 7 +++++++\n t/t0068-for-each-repo.sh | 6 ++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\nindex 5bba623ff12..52be64a4373 100644\n--- a/builtin/for-each-repo.c\n+++ b/builtin/for-each-repo.c\n@@ -51,6 +51,13 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n \tvalues = repo_config_get_value_multi(the_repository,\n \t\t\t\t\t     config_key);\n \n+\t/*\n+\t * Do nothing on an empty list, which is equivalent to the case\n+\t * where the config variable does not exist at all.\n+\t */\n+\tif (!values)\n+\t\treturn 0;\n+\n \tfor (i = 0; !result && i < values->nr; i++)\n \t\tresult = run_command_on_repo(values->items[i].string, &args);\n \ndiff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh\nindex 136b4ec8392..4675e852517 100755\n--- a/t/t0068-for-each-repo.sh\n+++ b/t/t0068-for-each-repo.sh\n@@ -27,4 +27,10 @@ test_expect_success 'run based on configured value' '\n \tgrep again message\n '\n \n+test_expect_success 'do nothing on empty config' '\n+\t# the whole thing would fail if for-each-ref iterated even\n+\t# once, because \"git help --no-such-option\" would fail\n+\tgit for-each-repo --config=bogus.config -- help --no-such-option\n+'\n+\n test_done\n\nbase-commit: 4950b2a2b5c4731e4c9d5b803739a6979b23fed6\n-- \ngitgitgadget\n"}]}