{"thread":{"id":"52436","subject":"[PATCH v2] config: add string mapping for enum config_scope","startedAt":"2019-12-11T23:38:27Z","lastAt":"2019-12-12T03:55:17Z","messageCount":7,"participants":["Emily Shaffer","Matt Rogers","Jeff King","mattr94@gmail.com"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"387994","messageId":"20191211233820.185153-1-emilyshaffer@google.com","threadId":"52436","inReplyTo":null,"subject":"[PATCH v2] config: add string mapping for enum config_scope","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-11T23:38:20Z","receivedAt":"2019-12-11T23:38:27Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"If a user is interacting with their config files primarily by the 'git\nconfig' command, using the location flags (--global, --system, etc) then\nthey may be more interested to see the scope of the config file they are\nediting, rather than the filepath.\n\nSigned-off-by: Emily Shaffer \n---\n\nSince v1, the only change is removing locale markers around the strings\nreturned from the helper.\n\nAs mentioned in lore.kernel.org/git/20191211232540.GE8464@google.com I'm\nstill not sure whether it's better to return \"local\" for\nCONFIG_SCOPE_REPO. Since that's the scope returned for both local and\nworktree (.git/config, .git/config.worktree) configs, I'm happy to leave\nit the way it is to indicate \"one of the configs in the repo\".\n\n - Emily\n\n config.c | 17 +++++++++++++++++\n config.h | 1 +\n 2 files changed, 18 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex e7052b3977..baab4a916e 100644\n--- a/config.c\n+++ b/config.c\n@@ -3312,6 +3312,23 @@ enum config_scope current_config_scope(void)\n \t\treturn current_parsing_scope;\n }\n \n+const char *config_scope_to_string(enum config_scope scope)\n+{\n+\tswitch (scope) {\n+\tcase CONFIG_SCOPE_SYSTEM:\n+\t\treturn \"system\";\n+\tcase CONFIG_SCOPE_GLOBAL:\n+\t\treturn \"global\";\n+\tcase CONFIG_SCOPE_REPO:\n+\t\treturn \"repo\";\n+\tcase CONFIG_SCOPE_CMDLINE:\n+\t\treturn \"cmdline\";\n+\tcase CONFIG_SCOPE_UNKNOWN:\n+\tdefault:\n+\t\treturn \"unknown\";\n+\t}\n+}\n+\n int lookup_config(const char **mapping, int nr_mapping, const char *var)\n {\n \tint i;\ndiff --git a/config.h b/config.h\nindex 91fd4c5e96..c8bf296dcc 100644\n--- a/config.h\n+++ b/config.h\n@@ -303,6 +303,7 @@ enum config_scope {\n };\n \n enum config_scope current_config_scope(void);\n+const char *config_scope_to_string(enum config_scope);\n const char *current_config_origin_type(void);\n const char *current_config_name(void);\n \n-- \n2.24.0.525.g8f36a354ae-goog\n\n"},{"id":"387995","messageId":"CAOjrSZsrNjzCojmA6kgSdhT9=G84twzv3Ced4VfNZD1SC2jb7Q@mail.gmail.com","threadId":"52436","inReplyTo":"20191211233820.185153-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] config: add string mapping for enum config_scope","fromName":"Matt Rogers","fromEmail":"mattr94@gmail.com","sentAt":"2019-12-11T23:52:10Z","receivedAt":"2019-12-11T23:52:24Z","isPatch":true,"sender":{"key":"mattr94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5719846?v=4"},"body":"On Wed, Dec 11, 2019 at 6:38 PM Emily Shaffer wrote:\n>\n> If a user is interacting with their config files primarily by the 'git\n> config' command, using the location flags (--global, --system, etc) then\n> they may be more interested to see the scope of the config file they are\n> editing, rather than the filepath.\n>\n> Signed-off-by: Emily Shaffer \n> ---\n>\n> Since v1, the only change is removing locale markers around the strings\n> returned from the helper.\n>\n> As mentioned in lore.kernel.org/git/20191211232540.GE8464@google.com I'm\n> still not sure whether it's better to return \"local\" for\n> CONFIG_SCOPE_REPO. Since that's the scope returned for both local and\n> worktree (.git/config, .git/config.worktree) configs, I'm happy to leave\n> it the way it is to indicate \"one of the configs in the repo\".\n>\n> - Emily\n>\n> config.c | 17 +++++++++++++++++\n> config.h | 1 +\n> 2 files changed, 18 insertions(+)\n>\n> diff --git a/config.c b/config.c\n> index e7052b3977..baab4a916e 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -3312,6 +3312,23 @@ enum config_scope current_config_scope(void)\n> return current_parsing_scope;\n> }\n>\n> +const char *config_scope_to_string(enum config_scope scope)\n> +{\n> + switch (scope) {\n> + case CONFIG_SCOPE_SYSTEM:\n> + return \"system\";\n> + case CONFIG_SCOPE_GLOBAL:\n> + return \"global\";\n> + case CONFIG_SCOPE_REPO:\n> + return \"repo\";\n> + case CONFIG_SCOPE_CMDLINE:\n> + return \"cmdline\";\n> + case CONFIG_SCOPE_UNKNOWN:\n> + default:\n> + return \"unknown\";\n> + }\n> +}\n> +\n> int lookup_config(const char **mapping, int nr_mapping, const char *var)\n> {\n> int i;\n> diff --git a/config.h b/config.h\n> index 91fd4c5e96..c8bf296dcc 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -303,6 +303,7 @@ enum config_scope {\n> };\n>\n> enum config_scope current_config_scope(void);\n> +const char *config_scope_to_string(enum config_scope);\n> const char *current_config_origin_type(void);\n> const char *current_config_name(void);\n>\n> --\n> 2.24.0.525.g8f36a354ae-goog\n>\n\nOkay Good to know, I'm using gmail right now, so the default is to\nstart at the top\n\nI wouldn't really consider it a bug so much as just something that\nhasn't been an issue before. The way config.c is set up kind of makes\nit hard to pass in the information that it uses the --local option, so\nthat may need to be refactored, since it just passes in the\nappropriate file based on the --local, etc. flags to builtin\\config.c\nwhich has never really needed that information before now. Anyways, I\ndon't think it needs to be addressed right now, and I'm working on\nsomething that would address it anyways... I just need to find some\nmore time to work on it.\n\n-- \nMatthew Rogers\n"},{"id":"388000","messageId":"20191212031003.GA1196215@coredump.intra.peff.net","threadId":"52436","inReplyTo":"20191211233820.185153-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] config: add string mapping for enum config_scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-12T03:10:03Z","receivedAt":"2019-12-12T03:10:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 11, 2019 at 03:38:20PM -0800, Emily Shaffer wrote:\n\n> If a user is interacting with their config files primarily by the 'git\n> config' command, using the location flags (--global, --system, etc) then\n> they may be more interested to see the scope of the config file they are\n> editing, rather than the filepath.\n\nI don't mind adding this in, but I think we did discuss this same\nconcept when we did \"config --show-origin\", and ended up showing the\nwhole path, to help with a few cases:\n\n - you know you're getting a value from the \"system\" config, but you\n don't know where that is (e.g., because the baked-in sysconfdir path\n is something you didn't expect)\n\n - you're in a scope like \"global\", but the value actually comes from\n an included file\n\nOf course there's a flip-side, which is that showing \"/etc/gitconfig\"\ndoesn't tell you that this is the \"--system\" file; the user has to infer\nthat themselves.\n\nThere are no callers added here, so I'm not sure exactly how the new\nfunction is meant to be used. But I'd caution that it might be worth\nshowing the scope _and_ the path, instead of one or the other.\n\n-Peff\n"},{"id":"388001","messageId":"1062901d5b09d$eafbc420$c0f34c60$@gmail.com","threadId":"52436","inReplyTo":"20191212031003.GA1196215@coredump.intra.peff.net","subject":"RE: [PATCH v2] config: add string mapping for enum config_scope","fromName":"","fromEmail":"mattr94@gmail.com","sentAt":"2019-12-12T03:40:37Z","receivedAt":"2019-12-12T03:40:43Z","isPatch":true,"sender":{"key":"mattr94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5719846?v=4"},"body":"From: Jeff King \nSent: Wednesday, December 11, 2019 10:10 PM\nTo: Emily Shaffer \nCc: git@vger.kernel.org; Matthew Rogers ; Philip Oakley \nSubject: Re: [PATCH v2] config: add string mapping for enum config_scope\n\nOn Wed, Dec 11, 2019 at 03:38:20PM -0800, Emily Shaffer wrote:\n\n> If a user is interacting with their config files primarily by the 'git \n> config' command, using the location flags (--global, --system, etc) \n> then they may be more interested to see the scope of the config file \n> they are editing, rather than the filepath.\n\nI don't mind adding this in, but I think we did discuss this same concept when we did \"config --show-origin\", and ended up showing the whole path, to help with a few cases:\n\n - you know you're getting a value from the \"system\" config, but you\n don't know where that is (e.g., because the baked-in sysconfdir path\n is something you didn't expect)\n\n - you're in a scope like \"global\", but the value actually comes from\n an included file\n\nOf course there's a flip-side, which is that showing \"/etc/gitconfig\"\ndoesn't tell you that this is the \"--system\" file; the user has to infer that themselves.\n\nThere are no callers added here, so I'm not sure exactly how the new function is meant to be used. But I'd caution that it might be worth showing the scope _and_ the path, instead of one or the other.\n\n-Peff\n\nJust to mention, that I am working on submitting a feature to expand config with a --show-scope option via gitgitgadget. I still have some kinks to iron out before it's ready for submission, but maybe it would make sense to wait for that? Or to wait for the rest of the patches this was taken from to actually materialize?\n\n"},{"id":"388002","messageId":"20191212034547.GA226275@google.com","threadId":"52436","inReplyTo":"20191212031003.GA1196215@coredump.intra.peff.net","subject":"Re: [PATCH v2] config: add string mapping for enum config_scope","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-12-12T03:45:47Z","receivedAt":"2019-12-12T03:45:54Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Dec 11, 2019 at 10:10:03PM -0500, Jeff King wrote:\n> On Wed, Dec 11, 2019 at 03:38:20PM -0800, Emily Shaffer wrote:\n> \n> > If a user is interacting with their config files primarily by the 'git\n> > config' command, using the location flags (--global, --system, etc) then\n> > they may be more interested to see the scope of the config file they are\n> > editing, rather than the filepath.\n> \n> I don't mind adding this in, but I think we did discuss this same\n> concept when we did \"config --show-origin\", and ended up showing the\n> whole path, to help with a few cases:\n> \n> - you know you're getting a value from the \"system\" config, but you\n> don't know where that is (e.g., because the baked-in sysconfdir path\n> is something you didn't expect)\n> \n> - you're in a scope like \"global\", but the value actually comes from\n> an included file\n> \n> Of course there's a flip-side, which is that showing \"/etc/gitconfig\"\n> doesn't tell you that this is the \"--system\" file; the user has to infer\n> that themselves.\n> \n> There are no callers added here, so I'm not sure exactly how the new\n> function is meant to be used. But I'd caution that it might be worth\n> showing the scope _and_ the path, instead of one or the other.\n\nYeah, I hear you - I had added this originally to the config-based hooks\ntopic to get an output like this:\n\n$ git hook --list pre-commit\n001\tglobal\t~/foo.sh\n002\tlocal\t~/bar.sh\n\nThat's a scenario where it might be handy to add the path, especially if\nit's coming in via an import, sure. (For brevity I think I'd want to\nturn it on via an argument.)\n\nAs I was working through the comments on v3 of git-bugreport, though, I\nsaw a request to add the origin of the configs - and that's a case where\nI don't necessarily want someone to see, say:\n\n[Selected Configs]\nuser.name (/home/emily/robot-revolution/stairclimber/.git/config) : Emily Shaffer\n\nwhen I mail that bugreport to the Git list.\n\nSo, I think I hear what you're saying - use wisely - but I think it's OK\nfor a user to say:\n\n printf(\"%s (%s): %s = %s\\n\",\n current_config_name(),\n\t config_scope_to_string(current_config_scope()),\n\t var,\n\t value);\n\nThat is, I don't think the right solution is to make\ncurrent_config_name() provide a stringification of\ncurrent_config_scope() as well.\n\nOr, I guess we can decide that the bugreport scenario is different\nenough that this helper should exist only there, and everybody else\nshould use current_config_name().\n\n - Emily\n"},{"id":"388003","messageId":"20191212034927.GA1200720@coredump.intra.peff.net","threadId":"52436","inReplyTo":"1062901d5b09d$eafbc420$c0f34c60$@gmail.com","subject":"Re: [PATCH v2] config: add string mapping for enum config_scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-12T03:49:27Z","receivedAt":"2019-12-12T03:49:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 11, 2019 at 10:40:37PM -0500, mattr94@gmail.com wrote:\n\n> Just to mention, that I am working on submitting a feature to expand\n> config with a --show-scope option via gitgitgadget.\n\nThat makes sense (hopefully it can be combined with --show-origin, too,\nto get all of the information).\n\n> I still have some kinks to iron out before it's ready for submission,\n> but maybe it would make sense to wait for that? Or to wait for the\n> rest of the patches this was taken from to actually materialize?\n\nIn general, yes, I think it makes sense for a patch like this to come as\npart of a larger topic, that all gets merged at once. That way we don't\nend up with unused cruft.\n\nIt sounds like the issue is that there are two topics that use this.\nThe simplest thing may be to have a common topic branch that both build\non, but with a note never to merge it directly (that last part would be\ndone by Junio, but the submitter would want to give that guidance along\nwith the patch).\n\nI _think_ that would be easy to handle with the way Junio handles the\ntopics, but he may correct me.\n\n-Peff\n"},{"id":"388004","messageId":"20191212035515.GB1200720@coredump.intra.peff.net","threadId":"52436","inReplyTo":"20191212034547.GA226275@google.com","subject":"Re: [PATCH v2] config: add string mapping for enum config_scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-12T03:55:15Z","receivedAt":"2019-12-12T03:55:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 11, 2019 at 07:45:47PM -0800, Emily Shaffer wrote:\n\n> > There are no callers added here, so I'm not sure exactly how the new\n> > function is meant to be used. But I'd caution that it might be worth\n> > showing the scope _and_ the path, instead of one or the other.\n> \n> Yeah, I hear you - I had added this originally to the config-based hooks\n> topic to get an output like this:\n> \n> $ git hook --list pre-commit\n> 001\tglobal\t~/foo.sh\n> 002\tlocal\t~/bar.sh\n> \n> That's a scenario where it might be handy to add the path, especially if\n> it's coming in via an import, sure. (For brevity I think I'd want to\n> turn it on via an argument.)\n\nYeah, there I think showing the file path would be helpful.\n\n> As I was working through the comments on v3 of git-bugreport, though, I\n> saw a request to add the origin of the configs - and that's a case where\n> I don't necessarily want someone to see, say:\n> \n> [Selected Configs]\n> user.name (/home/emily/robot-revolution/stairclimber/.git/config) : Emily Shaffer\n> \n> when I mail that bugreport to the Git list.\n\nYeah, agreed. You'd want less information there, because we should be\nsensitive to sharing user filesystem paths.\n\n> So, I think I hear what you're saying - use wisely - but I think it's OK\n> for a user to say:\n> \n> printf(\"%s (%s): %s = %s\\n\",\n> current_config_name(),\n> \t config_scope_to_string(current_config_scope()),\n> \t var,\n> \t value);\n> \n> That is, I don't think the right solution is to make\n> current_config_name() provide a stringification of\n> current_config_scope() as well.\n\nYes, I think the way you're thinking about it all makes sense. We\ndefinitely can't just change current_config_name(); its output ends up\nin --show-origin, which is plumbing.\n\n> Or, I guess we can decide that the bugreport scenario is different\n> enough that this helper should exist only there, and everybody else\n> should use current_config_name().\n\nNo, I think this helper makes sense in config.c. And then callers can\nchoose how much (and in what format) to show the various bits as\nappropriate.\n\nSo this seems like the right direction.\n\n-Peff\n"}]}