* [PATCH] refactor of git_setup_gettext method
@ 2021-05-30 21:42 Dima Kov via GitGitGadget
2021-05-30 22:14 ` Eric Sunshine
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Dima Kov via GitGitGadget @ 2021-05-30 21:42 UTC (permalink / raw)
To: git; +Cc: Dima Kov, Dima Kov
From: Dima Kov <dima.kovol@gmail.com>
Use one "free" call at the end of the function,
instead of being dependent on the execution flow.
Signed-off-by: Dima Kov <dima.kovol@gmail.com>
---
refactor of git_setup_gettext method
Use one "free" call at the end of the function, instead of being
dependent on the execution flow.
Signed-off-by: Dima Kov dima.kovol@gmail.com
Hi. My first PR fot Git repository. I went over the code and saw that
maybe this part can be more clearer. Thanks.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-964%2Fdimakov%2Ffree_opt-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-964/dimakov/free_opt-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/964
gettext.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/gettext.c b/gettext.c
index af2413b47e85..e75c7bfdb1a8 100644
--- a/gettext.c
+++ b/gettext.c
@@ -109,17 +109,14 @@ void git_setup_gettext(void)
if (!podir)
podir = p = system_path(GIT_LOCALE_PATH);
- if (!is_directory(podir)) {
- free(p);
- return;
+ if (is_directory(podir)) {
+ bindtextdomain("git", podir);
+ setlocale(LC_MESSAGES, "");
+ setlocale(LC_TIME, "");
+ init_gettext_charset("git");
+ textdomain("git");
}
- bindtextdomain("git", podir);
- setlocale(LC_MESSAGES, "");
- setlocale(LC_TIME, "");
- init_gettext_charset("git");
- textdomain("git");
-
free(p);
}
base-commit: de88ac70f3a801262eb3aa087e5d9a712be0a54a
--
gitgitgadget
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] refactor of git_setup_gettext method
2021-05-30 21:42 [PATCH] refactor of git_setup_gettext method Dima Kov via GitGitGadget
@ 2021-05-30 22:14 ` Eric Sunshine
2021-05-31 1:54 ` Junio C Hamano
2021-05-31 3:42 ` Bagas Sanjaya
2 siblings, 0 replies; 4+ messages in thread
From: Eric Sunshine @ 2021-05-30 22:14 UTC (permalink / raw)
To: Dima Kov via GitGitGadget; +Cc: Git List, Dima Kov
On Sun, May 30, 2021 at 5:43 PM Dima Kov via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Use one "free" call at the end of the function,
> instead of being dependent on the execution flow.
>
> Signed-off-by: Dima Kov <dima.kovol@gmail.com>
> ---
> Hi. My first PR fot Git repository. I went over the code and saw that
> maybe this part can be more clearer. Thanks.
Thanks for taking the time to submit a patch to the project. Your
change looks "obviously correct", however...
> diff --git a/gettext.c b/gettext.c
> @@ -109,17 +109,14 @@ void git_setup_gettext(void)
> - if (!is_directory(podir)) {
> - free(p);
> - return;
> + if (is_directory(podir)) {
> + bindtextdomain("git", podir);
> + setlocale(LC_MESSAGES, "");
> + setlocale(LC_TIME, "");
> + init_gettext_charset("git");
> + textdomain("git");
> }
>
> - bindtextdomain("git", podir);
> - setlocale(LC_MESSAGES, "");
> - setlocale(LC_TIME, "");
> - init_gettext_charset("git");
> - textdomain("git");
> -
> free(p);
... "clearness" is subjective, and it turns out that this project
generally prefers the code the way it was before this patch, and you
will find a lot of similar code throughout the project. In particular,
we like to get the simple cases and the error cases out of the way
early so that we don't have to think about them again for the
remainder of the function. Doing it this way eases cognitive load. (A
minor side benefit is that it also saves one or more levels of
indentation.)
An alternative which is also crops up somewhat often in this project
is to use `goto`, like this:
if (!is_directory(podir))
goto done;
bindtextdomain(...);
...
done:
free(p);
However, `goto` is most often used when there is a lot of cleanup to
do or the cleanup is intricate. This specific case doesn't qualify as
either, so the code is probably fine as-is.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] refactor of git_setup_gettext method
2021-05-30 21:42 [PATCH] refactor of git_setup_gettext method Dima Kov via GitGitGadget
2021-05-30 22:14 ` Eric Sunshine
@ 2021-05-31 1:54 ` Junio C Hamano
2021-05-31 3:42 ` Bagas Sanjaya
2 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2021-05-31 1:54 UTC (permalink / raw)
To: Dima Kov via GitGitGadget; +Cc: git, Dima Kov
"Dima Kov via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Dima Kov <dima.kovol@gmail.com>
> Subject: Re: [PATCH] refactor of git_setup_gettext method
We do not write in C++, so "method" -> "function".
> Use one "free" call at the end of the function,
> instead of being dependent on the execution flow.
>
> Signed-off-by: Dima Kov <dima.kovol@gmail.com>
> ---
I think an early return is more readable, but if this were a new
code and used the style used in this patch, it would also have been
acceptable. IOW, this is probably a borderline "Meh" change,
belonging to "what's already commited is good enough and it is not
worth the brain cycles to swap it around" category.
> gettext.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/gettext.c b/gettext.c
> index af2413b47e85..e75c7bfdb1a8 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -109,17 +109,14 @@ void git_setup_gettext(void)
> if (!podir)
> podir = p = system_path(GIT_LOCALE_PATH);
>
> - if (!is_directory(podir)) {
> - free(p);
> - return;
> + if (is_directory(podir)) {
> + bindtextdomain("git", podir);
> + setlocale(LC_MESSAGES, "");
> + setlocale(LC_TIME, "");
> + init_gettext_charset("git");
> + textdomain("git");
> }
>
> - bindtextdomain("git", podir);
> - setlocale(LC_MESSAGES, "");
> - setlocale(LC_TIME, "");
> - init_gettext_charset("git");
> - textdomain("git");
> -
> free(p);
> }
>
>
> base-commit: de88ac70f3a801262eb3aa087e5d9a712be0a54a
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] refactor of git_setup_gettext method
2021-05-30 21:42 [PATCH] refactor of git_setup_gettext method Dima Kov via GitGitGadget
2021-05-30 22:14 ` Eric Sunshine
2021-05-31 1:54 ` Junio C Hamano
@ 2021-05-31 3:42 ` Bagas Sanjaya
2 siblings, 0 replies; 4+ messages in thread
From: Bagas Sanjaya @ 2021-05-31 3:42 UTC (permalink / raw)
To: Dima Kov via GitGitGadget, git; +Cc: Dima Kov
Hi Dima, welcome to Git mailing list!
> diff --git a/gettext.c b/gettext.c
> index af2413b47e85..e75c7bfdb1a8 100644
> --- a/gettext.c
> +++ b/gettext.c
> @@ -109,17 +109,14 @@ void git_setup_gettext(void)
> if (!podir)
> podir = p = system_path(GIT_LOCALE_PATH);
>
> - if (!is_directory(podir)) {
> - free(p);
> - return;
> + if (is_directory(podir)) {
> + bindtextdomain("git", podir);
> + setlocale(LC_MESSAGES, "");
> + setlocale(LC_TIME, "");
> + init_gettext_charset("git");
> + textdomain("git");
> }
>
> - bindtextdomain("git", podir);
> - setlocale(LC_MESSAGES, "");
> - setlocale(LC_TIME, "");
> - init_gettext_charset("git");
> - textdomain("git");
> -
It seems like you move gettext initialization into if body, so the patch
title should be better say "move gettext initialization to if body".
> free(p);
> }
>
Thanks for review.
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-31 3:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-30 21:42 [PATCH] refactor of git_setup_gettext method Dima Kov via GitGitGadget
2021-05-30 22:14 ` Eric Sunshine
2021-05-31 1:54 ` Junio C Hamano
2021-05-31 3:42 ` Bagas Sanjaya
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).