{"thread":{"id":"60802","subject":"[PATCH 0/1] config: add back code comment","startedAt":"2024-01-28T18:32:50Z","lastAt":"2024-01-29T18:28:21Z","messageCount":6,"participants":["Kristoffer Haugsbakk","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"487470","messageId":"cover.1706466321.git.code@khaugsbakk.name","threadId":"60802","inReplyTo":null,"subject":"[PATCH 0/1] config: add back code comment","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-28T18:31:39Z","receivedAt":"2024-01-28T18:32:50Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"This is a follow-up to the kh/maintenance-use-xdg-when-it-should\n[series] which was merged in 12ee4ed506 (Merge branch\n'kh/maintenance-use-xdg-when-it-sho.., 2024-01-26).\n\nI dropped a code comment while iterating on a refactor. It still makes\nas much sense in this context as before the refactor (it’s a _refactor_\nin the sense of “don’t change code behavior”).\n\nThe code comment was moved to `config.c` in patch v1 3/4.[1] But review\nfeedback said that this comment didn’t fit in this new place and that we\nshouldn’t `die()` in `git_global_config`. So in v2 3/4[2] I removed the\ncomment in `git_global_config`. But I forgot to put the comment back to\nits original place, where it still makes as much sense as before my\nseries.\n\n[Here] is the diff when I squash this patch into c15129b699 (config:\nfactor out global config file retrieval, 2024-01-18).\n\nSorry about the churn.\n\nCc: ps@pks.im\n\n🔗 series: https://lore.kernel.org/git/cover.1697660181.git.code@khaugsbakk.name/\n🔗 1: https://lore.kernel.org/git/147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name/\n🔗 2: https://lore.kernel.org/git/32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name/\n† Here:\n\n    diff --git a/builtin/config.c b/builtin/config.c\n    index 6fff265581..b55bfae7d6 100644\n    --- a/builtin/config.c\n    +++ b/builtin/config.c\n    @@ -708,10 +708,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n            }\n\n            if (use_global_config) {\n    -\t\tchar *user_config, *xdg_config;\n    -\n    -\t\tgit_global_config_paths(&user_config, &xdg_config);\n    -\t\tif (!user_config)\n    +\t\tgiven_config_source.file = git_global_config();\n    +\t\tif (!given_config_source.file)\n                            /*\n                             * It is unknown if HOME/.gitconfig exists, so\n                             * we do not know if we should write to XDG\n    @@ -719,19 +717,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n                             * is set and points at a sane location.\n                             */\n                            die(_(\"$HOME not set\"));\n    -\n                    given_config_source.scope = CONFIG_SCOPE_GLOBAL;\n    -\n    -\t\tif (access_or_warn(user_config, R_OK, 0) &&\n    -\t\t    xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n    -\t\t\tgiven_config_source.file = xdg_config;\n    -\t\t\tfree(user_config);\n    -\t\t} else {\n    -\t\t\tgiven_config_source.file = user_config;\n    -\t\t\tfree(xdg_config);\n    -\t\t}\n    -\t}\n    -\telse if (use_system_config) {\n    +\t} else if (use_system_config) {\n                    given_config_source.file = git_system_config();\n                    given_config_source.scope = CONFIG_SCOPE_SYSTEM;\n            } else if (use_local_config) {\n    diff --git a/config.c b/config.c\n    index ebc6a57e1c..3cfeb3d8bd 100644\n    --- a/config.c\n    +++ b/config.c\n    @@ -1987,6 +1987,26 @@ char *git_system_config(void)\n            return system_config;\n     }\n\n    +char *git_global_config(void)\n    +{\n    +\tchar *user_config, *xdg_config;\n    +\n    +\tgit_global_config_paths(&user_config, &xdg_config);\n    +\tif (!user_config) {\n    +\t\tfree(xdg_config);\n    +\t\treturn NULL;\n    +\t}\n    +\n    +\tif (access_or_warn(user_config, R_OK, 0) && xdg_config &&\n    +\t    !access_or_warn(xdg_config, R_OK, 0)) {\n    +\t\tfree(user_config);\n    +\t\treturn xdg_config;\n    +\t} else {\n    +\t\tfree(xdg_config);\n    +\t\treturn user_config;\n    +\t}\n    +}\n    +\n     void git_global_config_paths(char **user_out, char **xdg_out)\n     {\n            char *user_config = xstrdup_or_null(getenv(\"GIT_CONFIG_GLOBAL\"));\n    diff --git a/config.h b/config.h\n    index e5e523553c..5dba984f77 100644\n    --- a/config.h\n    +++ b/config.h\n    @@ -382,6 +382,7 @@ int config_error_nonbool(const char *);\n     #endif\n\n     char *git_system_config(void);\n    +char *git_global_config(void);\n     void git_global_config_paths(char **user, char **xdg);\n\n     int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n\n\nKristoffer Haugsbakk (1):\n  config: add back code comment\n\n builtin/config.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\n-- \n2.43.0\n\n"},{"id":"487471","messageId":"48d66e94ece3b763acbe933561d82157c02a5f58.1706466321.git.code@khaugsbakk.name","threadId":"60802","inReplyTo":"cover.1706466321.git.code@khaugsbakk.name","subject":"[PATCH 1/1] config: add back code comment","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-28T18:31:40Z","receivedAt":"2024-01-28T18:32:54Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"c15129b699 (config: factor out global config file retrieval, 2024-01-18)\nwas a refactor that moved some of the code in this function to\n`config.c`. However, in the process I managed to drop this code comment\nwhich explains `$HOME not set`.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n builtin/config.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 08fe36d499..b55bfae7d6 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -710,6 +710,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (use_global_config) {\n \t\tgiven_config_source.file = git_global_config();\n \t\tif (!given_config_source.file)\n+\t\t\t/*\n+\t\t\t * It is unknown if HOME/.gitconfig exists, so\n+\t\t\t * we do not know if we should write to XDG\n+\t\t\t * location; error out even if XDG_CONFIG_HOME\n+\t\t\t * is set and points at a sane location.\n+\t\t\t */\n \t\t\tdie(_(\"$HOME not set\"));\n \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n \t} else if (use_system_config) {\n-- \n2.43.0\n\n"},{"id":"487508","messageId":"ZbeMw5tgY9S6k6y6@tanuki","threadId":"60802","inReplyTo":"48d66e94ece3b763acbe933561d82157c02a5f58.1706466321.git.code@khaugsbakk.name","subject":"Re: [PATCH 1/1] config: add back code comment","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:32:19Z","receivedAt":"2024-01-29T11:32:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 28, 2024 at 07:31:40PM +0100, Kristoffer Haugsbakk wrote:\n> c15129b699 (config: factor out global config file retrieval, 2024-01-18)\n> was a refactor that moved some of the code in this function to\n> `config.c`. However, in the process I managed to drop this code comment\n> which explains `$HOME not set`.\n> \n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n>  builtin/config.c | 6 ++++++\n>  1 file changed, 6 insertions(+)\n> \n> diff --git a/builtin/config.c b/builtin/config.c\n> index 08fe36d499..b55bfae7d6 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -710,6 +710,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>  \tif (use_global_config) {\n>  \t\tgiven_config_source.file = git_global_config();\n>  \t\tif (!given_config_source.file)\n> +\t\t\t/*\n> +\t\t\t * It is unknown if HOME/.gitconfig exists, so\n> +\t\t\t * we do not know if we should write to XDG\n> +\t\t\t * location; error out even if XDG_CONFIG_HOME\n> +\t\t\t * is set and points at a sane location.\n> +\t\t\t */\n>  \t\t\tdie(_(\"$HOME not set\"));\n>  \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n>  \t} else if (use_system_config) {\n\nThanks for adding the comment back in! The patch looks good to me.\n\nPatrick\n"},{"id":"487532","messageId":"cover.1706550761.git.code@khaugsbakk.name","threadId":"60802","inReplyTo":"48d66e94ece3b763acbe933561d82157c02a5f58.1706466321.git.code@khaugsbakk.name","subject":"[PATCH v2 0/1] config: add back code comment","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-29T17:57:50Z","receivedAt":"2024-01-29T17:58:41Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"This is a follow-up to the kh/maintenance-use-xdg-when-it-should\n[series] which was merged in 12ee4ed506 (Merge branch\n'kh/maintenance-use-xdg-when-it-sho.., 2024-01-26).\n\nI dropped a code comment while iterating on a refactor. It still makes\nas much sense in this context as before the refactor (it’s a _refactor_\nin the sense of “don’t change code behavior”).\n\nThe code comment was moved to `config.c` in patch v1 3/4.[1] But review\nfeedback said that this comment didn’t fit in this new place and that we\nshouldn’t `die()` in `git_global_config`. So in v2 3/4[2] I removed the\ncomment in `git_global_config`. But I forgot to put the comment back to\nits original place, where it still makes as much sense as before my\nseries.\n\nSee the cover letter on the first version for the diff when I squash\nthis patch into c15129b699 (config: factor out global config file\nretrieval, 2024-01-18).\n\nSorry about the churn.\n\nCc: ps@pks.im\n\n§ Changes in v2\n\nAdd an ack trailer.\n\nThis is the (tentative) final version. I read (interpreted)\n`SubmittingPatches` as saying that the final version should be sent,\neven though it’s just to add an additional trailer. I’m open for\nfeedback on the submission process of course.\n\nI’ve added it after my signoff since it seems preferred to maintain the\nchronology (although in this case either choice seems equally\nclear). Also it seemed more common in the recent Git log.\n\n🔗 series: https://lore.kernel.org/git/cover.1697660181.git.code@khaugsbakk.name/\n🔗 1: https://lore.kernel.org/git/147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name/\n🔗 2: https://lore.kernel.org/git/32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name/\n\nKristoffer Haugsbakk (1):\n  config: add back code comment\n\n builtin/config.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\nRange-diff against v1:\n1:  48d66e94ec ! 1:  24f536d575 config: add back code comment\n    @@ Commit message\n         which explains `$HOME not set`.\n     \n         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n    +    Acked-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## builtin/config.c ##\n     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)\n-- \n2.43.0\n\n"},{"id":"487533","messageId":"24f536d575d508b0784e0adf647cb2334f6704d8.1706550761.git.code@khaugsbakk.name","threadId":"60802","inReplyTo":"cover.1706550761.git.code@khaugsbakk.name","subject":"[PATCH v2 1/1] config: add back code comment","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-01-29T17:57:51Z","receivedAt":"2024-01-29T17:58:45Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"c15129b699 (config: factor out global config file retrieval, 2024-01-18)\nwas a refactor that moved some of the code in this function to\n`config.c`. However, in the process I managed to drop this code comment\nwhich explains `$HOME not set`.\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\nAcked-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/config.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 08fe36d499..b55bfae7d6 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -710,6 +710,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (use_global_config) {\n \t\tgiven_config_source.file = git_global_config();\n \t\tif (!given_config_source.file)\n+\t\t\t/*\n+\t\t\t * It is unknown if HOME/.gitconfig exists, so\n+\t\t\t * we do not know if we should write to XDG\n+\t\t\t * location; error out even if XDG_CONFIG_HOME\n+\t\t\t * is set and points at a sane location.\n+\t\t\t */\n \t\t\tdie(_(\"$HOME not set\"));\n \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n \t} else if (use_system_config) {\n-- \n2.43.0\n\n"},{"id":"487538","messageId":"xmqqil3chu4f.fsf@gitster.g","threadId":"60802","inReplyTo":"ZbeMw5tgY9S6k6y6@tanuki","subject":"Re: [PATCH 1/1] config: add back code comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T18:28:16Z","receivedAt":"2024-01-29T18:28:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>>  \t\tif (!given_config_source.file)\n>> +\t\t\t/*\n>> +\t\t\t * It is unknown if HOME/.gitconfig exists, so\n>> +\t\t\t * we do not know if we should write to XDG\n>> +\t\t\t * location; error out even if XDG_CONFIG_HOME\n>> +\t\t\t * is set and points at a sane location.\n>> +\t\t\t */\n>>  \t\t\tdie(_(\"$HOME not set\"));\n>>  \t\tgiven_config_source.scope = CONFIG_SCOPE_GLOBAL;\n>>  \t} else if (use_system_config) {\n>\n> Thanks for adding the comment back in! The patch looks good to me.\n\nYeah, thanks, both.\n"}]}