{"thread":{"id":"55806","subject":"[PATCH] refactor of git_setup_gettext method","startedAt":"2021-05-30T21:42:48Z","lastAt":"2021-05-31T03:42:53Z","messageCount":4,"participants":["Dima Kov via GitGitGadget","Eric Sunshine","Junio C Hamano","Bagas Sanjaya"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"425940","messageId":"pull.964.git.1622410962551.gitgitgadget@gmail.com","threadId":"55806","inReplyTo":null,"subject":"[PATCH] refactor of git_setup_gettext method","fromName":"Dima Kov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-30T21:42:42Z","receivedAt":"2021-05-30T21:42:48Z","isPatch":true,"sender":{"key":"name:Dima Kov","avatar":null},"body":"From: Dima Kov \n\nUse one \"free\" call at the end of the function,\ninstead of being dependent on the execution flow.\n\nSigned-off-by: Dima Kov \n---\n refactor of git_setup_gettext method\n \n Use one \"free\" call at the end of the function, instead of being\n dependent on the execution flow.\n \n Signed-off-by: Dima Kov dima.kovol@gmail.com\n \n Hi. My first PR fot Git repository. I went over the code and saw that\n maybe this part can be more clearer. Thanks.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-964%2Fdimakov%2Ffree_opt-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-964/dimakov/free_opt-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/964\n\n gettext.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/gettext.c b/gettext.c\nindex af2413b47e85..e75c7bfdb1a8 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -109,17 +109,14 @@ void git_setup_gettext(void)\n \tif (!podir)\n \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n \n-\tif (!is_directory(podir)) {\n-\t\tfree(p);\n-\t\treturn;\n+\tif (is_directory(podir)) {\n+\t\tbindtextdomain(\"git\", podir);\n+\t\tsetlocale(LC_MESSAGES, \"\");\n+\t\tsetlocale(LC_TIME, \"\");\n+\t\tinit_gettext_charset(\"git\");\n+\t\ttextdomain(\"git\");\n \t}\n \n-\tbindtextdomain(\"git\", podir);\n-\tsetlocale(LC_MESSAGES, \"\");\n-\tsetlocale(LC_TIME, \"\");\n-\tinit_gettext_charset(\"git\");\n-\ttextdomain(\"git\");\n-\n \tfree(p);\n }\n \n\nbase-commit: de88ac70f3a801262eb3aa087e5d9a712be0a54a\n-- \ngitgitgadget\n"},{"id":"425945","messageId":"CAPig+cQdfadxF-AJAFtOv2XTjoLKQ1DqCfvDm-mtqPSj6eMrcA@mail.gmail.com","threadId":"55806","inReplyTo":"pull.964.git.1622410962551.gitgitgadget@gmail.com","subject":"Re: [PATCH] refactor of git_setup_gettext method","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-05-30T22:14:17Z","receivedAt":"2021-05-30T22:14:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, May 30, 2021 at 5:43 PM Dima Kov via GitGitGadget\n wrote:\n> Use one \"free\" call at the end of the function,\n> instead of being dependent on the execution flow.\n>\n> Signed-off-by: Dima Kov \n> ---\n> Hi. My first PR fot Git repository. I went over the code and saw that\n> maybe this part can be more clearer. Thanks.\n\nThanks for taking the time to submit a patch to the project. Your\nchange looks \"obviously correct\", however...\n\n> diff --git a/gettext.c b/gettext.c\n> @@ -109,17 +109,14 @@ void git_setup_gettext(void)\n> - if (!is_directory(podir)) {\n> - free(p);\n> - return;\n> + if (is_directory(podir)) {\n> + bindtextdomain(\"git\", podir);\n> + setlocale(LC_MESSAGES, \"\");\n> + setlocale(LC_TIME, \"\");\n> + init_gettext_charset(\"git\");\n> + textdomain(\"git\");\n> }\n>\n> - bindtextdomain(\"git\", podir);\n> - setlocale(LC_MESSAGES, \"\");\n> - setlocale(LC_TIME, \"\");\n> - init_gettext_charset(\"git\");\n> - textdomain(\"git\");\n> -\n> free(p);\n\n... \"clearness\" is subjective, and it turns out that this project\ngenerally prefers the code the way it was before this patch, and you\nwill find a lot of similar code throughout the project. In particular,\nwe like to get the simple cases and the error cases out of the way\nearly so that we don't have to think about them again for the\nremainder of the function. Doing it this way eases cognitive load. (A\nminor side benefit is that it also saves one or more levels of\nindentation.)\n\nAn alternative which is also crops up somewhat often in this project\nis to use `goto`, like this:\n\n if (!is_directory(podir))\n goto done;\n bindtextdomain(...);\n ...\n done:\n free(p);\n\nHowever, `goto` is most often used when there is a lot of cleanup to\ndo or the cleanup is intricate. This specific case doesn't qualify as\neither, so the code is probably fine as-is.\n"},{"id":"425951","messageId":"xmqqbl8r1xck.fsf@gitster.g","threadId":"55806","inReplyTo":"pull.964.git.1622410962551.gitgitgadget@gmail.com","subject":"Re: [PATCH] refactor of git_setup_gettext method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-31T01:54:51Z","receivedAt":"2021-05-31T01:54:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dima Kov via GitGitGadget\" writes:\n\n> From: Dima Kov \n> Subject: Re: [PATCH] refactor of git_setup_gettext method\n\nWe do not write in C++, so \"method\" -> \"function\".\n\n> Use one \"free\" call at the end of the function,\n> instead of being dependent on the execution flow.\n>\n> Signed-off-by: Dima Kov \n> ---\n\nI think an early return is more readable, but if this were a new\ncode and used the style used in this patch, it would also have been\nacceptable. IOW, this is probably a borderline \"Meh\" change,\nbelonging to \"what's already commited is good enough and it is not\nworth the brain cycles to swap it around\" category.\n\n\n> gettext.c | 15 ++++++---------\n> 1 file changed, 6 insertions(+), 9 deletions(-)\n>\n> diff --git a/gettext.c b/gettext.c\n> index af2413b47e85..e75c7bfdb1a8 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -109,17 +109,14 @@ void git_setup_gettext(void)\n> \tif (!podir)\n> \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n> \n> -\tif (!is_directory(podir)) {\n> -\t\tfree(p);\n> -\t\treturn;\n> +\tif (is_directory(podir)) {\n> +\t\tbindtextdomain(\"git\", podir);\n> +\t\tsetlocale(LC_MESSAGES, \"\");\n> +\t\tsetlocale(LC_TIME, \"\");\n> +\t\tinit_gettext_charset(\"git\");\n> +\t\ttextdomain(\"git\");\n> \t}\n> \n> -\tbindtextdomain(\"git\", podir);\n> -\tsetlocale(LC_MESSAGES, \"\");\n> -\tsetlocale(LC_TIME, \"\");\n> -\tinit_gettext_charset(\"git\");\n> -\ttextdomain(\"git\");\n> -\n> \tfree(p);\n> }\n> \n>\n> base-commit: de88ac70f3a801262eb3aa087e5d9a712be0a54a\n"},{"id":"425955","messageId":"4df3a985-1359-cec9-c3db-fe069f24297d@gmail.com","threadId":"55806","inReplyTo":"pull.964.git.1622410962551.gitgitgadget@gmail.com","subject":"Re: [PATCH] refactor of git_setup_gettext method","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-05-31T03:42:21Z","receivedAt":"2021-05-31T03:42:53Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"Hi Dima, welcome to Git mailing list!\n\n> diff --git a/gettext.c b/gettext.c\n> index af2413b47e85..e75c7bfdb1a8 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -109,17 +109,14 @@ void git_setup_gettext(void)\n> \tif (!podir)\n> \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n> \n> -\tif (!is_directory(podir)) {\n> -\t\tfree(p);\n> -\t\treturn;\n> +\tif (is_directory(podir)) {\n> +\t\tbindtextdomain(\"git\", podir);\n> +\t\tsetlocale(LC_MESSAGES, \"\");\n> +\t\tsetlocale(LC_TIME, \"\");\n> +\t\tinit_gettext_charset(\"git\");\n> +\t\ttextdomain(\"git\");\n> \t}\n> \n> -\tbindtextdomain(\"git\", podir);\n> -\tsetlocale(LC_MESSAGES, \"\");\n> -\tsetlocale(LC_TIME, \"\");\n> -\tinit_gettext_charset(\"git\");\n> -\ttextdomain(\"git\");\n> -\n\nIt seems like you move gettext initialization into if body, so the patch \ntitle should be better say \"move gettext initialization to if body\".\n\n> \tfree(p);\n> }\n> \n\nThanks for review.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"}]}