Re: [PATCH v3] git-merge: rewrite already up to date message

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 22, 2021, 03:41 UTC
Message-ID: <CAPig+cTZRM1d07Nd0WBtWm5AO1mfh9M8jvYAXcoPm9cJ1MpDnA@mail.gmail.com>
In-Reply-To: <pull.934.v3.git.1619052906768.gitgitgadget@gmail.com>

On Wed, Apr 21, 2021 at 8:55 PM Josh Soref via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 16 quoted lines

> Usually, it is easier to read a message if it makes its primary
> point first, before giving a parenthetical note.
> [...]
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
>     Changes since v2:
>
>      * finish_up_to_date now figures out the answer on its own to address
>        feedback from Eric Sunshine
>
>     -- Yes, I'm well aware of localization rules. But as Eric Sunshine
>     noted, the code was already making a mess of things. I didn't want to
>     make invasive changes. I actually wrote roughly what Eric proposed as an
>     earlier implementation, but the various complexities, including trying
>     to figure out what the yeah was for and who cared about it, made me
>     really wary of proposing such a significant change.

Understandable. I also was curious as to whether "Yeeah!" had any significance, thus checked the project history before responding to your v2. As far as I can tell, "Yeeah!" has no particular significance. It materialized out of thin air with 1c7b76be7d (Build in merge, 2008-07-07) and simply hasn't been touched since then (in any meaningful way). Delving into the list archive doesn't shed any additional light on "Yeeah!"; none of the reviews mentioned it at all. So, it doesn't seem to serve any genuine purpose, and I don't mind seeing it go away, especially since its removal simplifies things for translators.

Show 22 quoted lines

> diff --git a/builtin/merge.c b/builtin/merge.c
> @@ -380,10 +380,14 @@ static void restore_state(const struct object_id *head,
>  /* This is called when no merge was necessary. */
> -static void finish_up_to_date(const char *msg)
> +static void finish_up_to_date(void)
>  {
> -       if (verbosity >= 0)
> -               printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
> +       if (verbosity >= 0) {
> +               if (squash)
> +                       puts(_("Already up to date (nothing to squash)."));
> +               else
> +                       puts(_("Already up to date."));
> +       }
>         remove_merge_branch_state(the_repository);
>  }
> @@ -1482,7 +1486,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> -               finish_up_to_date(_("Already up to date."));
> +               finish_up_to_date();
> @@ -1566,7 +1570,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> -                       finish_up_to_date(_("Already up to date. Yeeah!"));
> +                       finish_up_to_date();

Perhaps not surprisingly, I find this version of the patch much easier to understand.

Thanks for re-rolling.