Re: [PATCH] refactor of git_setup_gettext method

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 30, 2021, 22:14 UTC
Message-ID: <CAPig+cQdfadxF-AJAFtOv2XTjoLKQ1DqCfvDm-mtqPSj6eMrcA@mail.gmail.com>
In-Reply-To: <pull.964.git.1622410962551.gitgitgadget@gmail.com>

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.