threads / patch / 55806

patchrefactor of git_setup_gettext method

Subject: [PATCH] refactor of git_setup_gettext method

## tl;dr

4 messages between May 30, 2021 and May 31, 2021. Diffs are folded; open one to read it.

replies: 3people: 4as markdown or json

Dima Kov via GitGitGadget· May 30, 2021, 21:42 UTC · lore

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(-)

Show changes to gettext.c +6 −9

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

Eric Sunshine· May 30, 2021, 22:14 UTC · re: Dima Kov via GitGitGadget · lore

Re: [PATCH] refactor of git_setup_gettext method

On Sun, May 30, 2021 at 5:43 PM Dima Kov via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 7 quoted lines

> 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...

Show 20 quoted lines

> 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.

Junio C Hamano· May 31, 2021, 01:54 UTC · re: Dima Kov via GitGitGadget · lore

Re: [PATCH] refactor of git_setup_gettext method

"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".

Show 5 quoted lines

> 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.

Show 33 quoted lines

>  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

Bagas Sanjaya· May 31, 2021, 03:42 UTC · re: Dima Kov via GitGitGadget · lore

Re: [PATCH] refactor of git_setup_gettext method

Hi Dima, welcome to Git mailing list!

Show 25 quoted lines

> 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