[PATCH] git-merge: move space to between strings

[PATCH] git-merge: move space to between strings

[Josh Soref via GitGitGadget]

From: Josh Soref <jsoref@gmail.com>

Signed-off-by: Josh Soref <jsoref@gmail.com>
---
    git-merge: move space to between strings
    
    GitHub Actions show things like:
    
     * branch                  master     -> FETCH_HEAD
     (nothing to squash)Already up to date.
    
    
    The expected results are:
    
     * branch                  master     -> FETCH_HEAD
    (nothing to squash) Already up to date.
    
    
    This commit should change that. Other than breaking all the
    localizations, and anyone who actively parses the output, this shouldn't
    have much impact.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-934%2Fjsoref%2Fnothing-to-squash-already-up-to-date-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-934/jsoref/nothing-to-squash-already-up-to-date-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/934

 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 062e91144125..0d8c782cccb2 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -383,7 +383,7 @@ static void restore_state(const struct object_id *head,
 static void finish_up_to_date(const char *msg)
 {
 	if (verbosity >= 0)
-		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
+		printf("%s%s\n", squash ? _("(nothing to squash) ") : "", msg);
 	remove_merge_branch_state(the_repository);
 }
 

base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0
-- 
gitgitgadget

Re: [PATCH] git-merge: move space to between strings

[Junio C Hamano]

"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Josh Soref <jsoref@gmail.com>
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
>     git-merge: move space to between strings
>     
>     GitHub Actions show things like:
>     
>      * branch                  master     -> FETCH_HEAD
>      (nothing to squash)Already up to date.
>     
>     
>     The expected results are:
>     
>      * branch                  master     -> FETCH_HEAD
>     (nothing to squash) Already up to date.

I am not sure if that is THE expected results, though (you wouldn't
have got this reaction if you said "I would expect to see").  

Usually, it is easier to read a message if it makes its primary
point first, before giving a parenthetical note.  I.e.  I would
expect that

	Already up to date (nothing to squash).

would be easier to understand to users.

Thanks.

[PATCH v2] git-merge: move primary point before parenthetical

[Josh Soref via GitGitGadget]

From: Josh Soref <jsoref@gmail.com>

Usually, it is easier to read a message if it makes its primary
point first, before giving a parenthetical note.

Before:
` (nothing to squash)Already up to date.
`

After:
`Already up to date (nothing to squash).
`

Signed-off-by: Josh Soref <jsoref@gmail.com>
---
    git-merge: move space to between strings
    
    GitHub Actions show things like:
    
     * branch                  master     -> FETCH_HEAD
     (nothing to squash)Already up to date.
    
    
    Usually, it is easier to read a message if it makes its primary point
    first, before giving a parenthetical note.
    
    The expected results are:
    
     * branch                  master     -> FETCH_HEAD
    Already up to date (nothing to squash).
    
    
    This commit should change that. Other than breaking all the
    localizations, and anyone who actively parses the output, this shouldn't
    have much impact.
    
    Changes since v1:
    
     * finish_up_to_date now takes a message with a %s for the parenthetical
       and a trailing \n to address feedback from Junio C Hamano

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-934%2Fjsoref%2Fnothing-to-squash-already-up-to-date-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-934/jsoref/nothing-to-squash-already-up-to-date-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/934

Range-diff vs v1:

 1:  1b9d685d611f ! 1:  4f60e08195ea git-merge: move space to between strings
     @@ Metadata
      Author: Josh Soref <jsoref@gmail.com>
      
       ## Commit message ##
     -    git-merge: move space to between strings
     +    git-merge: move primary point before parenthetical
     +
     +    Usually, it is easier to read a message if it makes its primary
     +    point first, before giving a parenthetical note.
     +
     +    Before:
     +    ` (nothing to squash)Already up to date.
     +    `
     +
     +    After:
     +    `Already up to date (nothing to squash).
     +    `
      
          Signed-off-by: Josh Soref <jsoref@gmail.com>
      
     @@ builtin/merge.c: static void restore_state(const struct object_id *head,
       {
       	if (verbosity >= 0)
      -		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
     -+		printf("%s%s\n", squash ? _("(nothing to squash) ") : "", msg);
     ++		printf(msg, squash ? _(" (nothing to squash)") : "");
       	remove_merge_branch_state(the_repository);
       }
       
     +@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
     + 		 * If head can reach all the merge then we are up to date.
     + 		 * but first the most common case of merging one remote.
     + 		 */
     +-		finish_up_to_date(_("Already up to date."));
     ++		finish_up_to_date(_("Already up to date%s.\n"));
     + 		goto done;
     + 	} else if (fast_forward != FF_NO && !remoteheads->next &&
     + 			!common->next &&
     +@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
     + 			}
     + 		}
     + 		if (up_to_date) {
     +-			finish_up_to_date(_("Already up to date. Yeeah!"));
     ++			finish_up_to_date(_("Already up to date%s. Yeeah!\n"));
     + 			goto done;
     + 		}
     + 	}


 builtin/merge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 062e91144125..aad180010670 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -383,7 +383,7 @@ static void restore_state(const struct object_id *head,
 static void finish_up_to_date(const char *msg)
 {
 	if (verbosity >= 0)
-		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
+		printf(msg, squash ? _(" (nothing to squash)") : "");
 	remove_merge_branch_state(the_repository);
 }
 
@@ -1482,7 +1482,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * If head can reach all the merge then we are up to date.
 		 * but first the most common case of merging one remote.
 		 */
-		finish_up_to_date(_("Already up to date."));
+		finish_up_to_date(_("Already up to date%s.\n"));
 		goto done;
 	} else if (fast_forward != FF_NO && !remoteheads->next &&
 			!common->next &&
@@ -1566,7 +1566,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			}
 		}
 		if (up_to_date) {
-			finish_up_to_date(_("Already up to date. Yeeah!"));
+			finish_up_to_date(_("Already up to date%s. Yeeah!\n"));
 			goto done;
 		}
 	}

base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0
-- 
gitgitgadget

Re: [PATCH v2] git-merge: move primary point before parenthetical

[Eric Sunshine]

On Wed, Apr 21, 2021 at 7:22 PM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Usually, it is easier to read a message if it makes its primary
> point first, before giving a parenthetical note.
>
> Before:
> ` (nothing to squash)Already up to date.
> `
>
> After:
> `Already up to date (nothing to squash).
> `

Thanks. This makes perfect sense and fixes what clearly seems to be a
case of swapped arguments to printf(). While the intent of the patch
is quite desirable, I do have some concerns about the actual
changes...

> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> diff --git a/builtin/merge.c b/builtin/merge.c
> @@ -383,7 +383,7 @@ static void restore_state(const struct object_id *head,
>  static void finish_up_to_date(const char *msg)
>  {
>         if (verbosity >= 0)
> -               printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
> +               printf(msg, squash ? _(" (nothing to squash)") : "");
>         remove_merge_branch_state(the_repository);
>  }
> @@ -1482,7 +1482,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> -               finish_up_to_date(_("Already up to date."));
> +               finish_up_to_date(_("Already up to date%s.\n"));
> @@ -1566,7 +1566,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(_("Already up to date%s. Yeeah!\n"));

I see why you changed the calling convention, making it the
responsibility of the caller to supply the "%s" so that the "(nothing
to squash)" annotation gets inserted before the period at the end of
"Already up to date". However, this makes things confusing for people
translating the text to other languages since they won't have enough
context to understand what gets interpolated in place of "%s".

In fact (not a fault of your patch), this sort of "sentence Lego" was
already a bit difficult on translators because they couldn't see the
entire context of "Already up to date" and "(nothing to squash)"
together, thus could only translate them individually which may have
led to inferior translations in some languages.

Ideally, for the sake of translators, we want to give them as much
context as possible, such as the entire "Already up to date (nothing
to squash)". So, keeping translators and context in mind, and asking
if we really need to that rather odd "Yeeah!", an alternative way to
resolve this problem would be like this:

    static void finish_up_to_date()
    {
        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);
    }

And then the callers reduce simply to:

    finish_up_to_date();

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

[Josh Soref via GitGitGadget]

From: Josh Soref <jsoref@gmail.com>

Usually, it is easier to read a message if it makes its primary
point first, before giving a parenthetical note.

Possible messages before include:
` (nothing to squash)Already up to date.
`
and
`Already up to date. Yeeah!
`

After:
`Already up to date (nothing to squash).
`
and
`Already up to date.
`

Localizations now have two easy to understand translatable strings.
(All localizations of the previous strings are broken.)

Co-authored-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Josh Soref <jsoref@gmail.com>
---
    git-merge: rewrite already up to date message
    
    GitHub Actions show things like:
    
     * branch                  master     -> FETCH_HEAD
     (nothing to squash)Already up to date.
    
    
    There was a code path where it would say:
    
    Already up to date. Yeeah!
    
    
    It is believed that other than being troublesome for localizers, this
    message added no value, so it's being removed.
    
    Usually, it is easier to read a message if it makes its primary point
    first, before giving a parenthetical note.
    
    The expected results are:
    
     * branch                  master     -> FETCH_HEAD
    Already up to date (nothing to squash).
    
    
    As well as an easier chance for localizers to actually translate the now
    two messages. (As they don't have to fight string pasting. Which is a
    localization sin.): Already up to date. Already up to date (nothing to
    squash).
    
    This commit should change that. Other than breaking anyone who actively
    parses the output and all the localizations (and giving localizers a
    real chance at localizing these messages), this shouldn't have much
    impact.
    
    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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-934%2Fjsoref%2Fnothing-to-squash-already-up-to-date-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-934/jsoref/nothing-to-squash-already-up-to-date-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/934

Range-diff vs v2:

 1:  4f60e08195ea ! 1:  a6c703603cda git-merge: move primary point before parenthetical
     @@ Metadata
      Author: Josh Soref <jsoref@gmail.com>
      
       ## Commit message ##
     -    git-merge: move primary point before parenthetical
     +    git-merge: rewrite already up to date message
      
          Usually, it is easier to read a message if it makes its primary
          point first, before giving a parenthetical note.
      
     -    Before:
     +    Possible messages before include:
          ` (nothing to squash)Already up to date.
          `
     +    and
     +    `Already up to date. Yeeah!
     +    `
      
          After:
          `Already up to date (nothing to squash).
          `
     +    and
     +    `Already up to date.
     +    `
     +
     +    Localizations now have two easy to understand translatable strings.
     +    (All localizations of the previous strings are broken.)
      
     +    Co-authored-by: Eric Sunshine <sunshine@sunshineco.com>
          Signed-off-by: Josh Soref <jsoref@gmail.com>
      
       ## builtin/merge.c ##
      @@ builtin/merge.c: static void restore_state(const struct object_id *head,
     - static void finish_up_to_date(const char *msg)
     + }
     + 
     + /* 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)
     +-	if (verbosity >= 0)
      -		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
     -+		printf(msg, squash ? _(" (nothing to squash)") : "");
     ++	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);
       }
       
     @@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
       		 * but first the most common case of merging one remote.
       		 */
      -		finish_up_to_date(_("Already up to date."));
     -+		finish_up_to_date(_("Already up to date%s.\n"));
     ++		finish_up_to_date();
       		goto done;
       	} else if (fast_forward != FF_NO && !remoteheads->next &&
       			!common->next &&
     @@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
       		}
       		if (up_to_date) {
      -			finish_up_to_date(_("Already up to date. Yeeah!"));
     -+			finish_up_to_date(_("Already up to date%s. Yeeah!\n"));
     ++			finish_up_to_date();
       			goto done;
       		}
       	}


 builtin/merge.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 062e91144125..f8c3d0687eaf 100644
--- 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)
 		 * If head can reach all the merge then we are up to date.
 		 * but first the most common case of merging one remote.
 		 */
-		finish_up_to_date(_("Already up to date."));
+		finish_up_to_date();
 		goto done;
 	} else if (fast_forward != FF_NO && !remoteheads->next &&
 			!common->next &&
@@ -1566,7 +1570,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			}
 		}
 		if (up_to_date) {
-			finish_up_to_date(_("Already up to date. Yeeah!"));
+			finish_up_to_date();
 			goto done;
 		}
 	}

base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0
-- 
gitgitgadget

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

[Eric Sunshine]

On Wed, Apr 21, 2021 at 8:55 PM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.

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

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

[Junio C Hamano]

"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Josh Soref <jsoref@gmail.com>
>
> Usually, it is easier to read a message if it makes its primary
> point first, before giving a parenthetical note.
>
> Possible messages before include:
> ` (nothing to squash)Already up to date.
> `
> and
> `Already up to date. Yeeah!
> `
>
> After:
> `Already up to date (nothing to squash).
> `
> and
> `Already up to date.
> `
>
> Localizations now have two easy to understand translatable strings.
> (All localizations of the previous strings are broken.)
>
> Co-authored-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---

I am not sure why this is Co-au, and not the more usual "Helped-by".

The patch text makes sense to me.

Thanks.


> diff --git a/builtin/merge.c b/builtin/merge.c
> index 062e91144125..f8c3d0687eaf 100644
> --- 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)
>  		 * If head can reach all the merge then we are up to date.
>  		 * but first the most common case of merging one remote.
>  		 */
> -		finish_up_to_date(_("Already up to date."));
> +		finish_up_to_date();
>  		goto done;
>  	} else if (fast_forward != FF_NO && !remoteheads->next &&
>  			!common->next &&
> @@ -1566,7 +1570,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  			}
>  		}
>  		if (up_to_date) {
> -			finish_up_to_date(_("Already up to date. Yeeah!"));
> +			finish_up_to_date();
>  			goto done;
>  		}
>  	}
>
> base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Josh Soref <jsoref@gmail.com>
>>
>> Usually, it is easier to read a message if it makes its primary
>> point first, before giving a parenthetical note.
>>
>> Possible messages before include:
>> ` (nothing to squash)Already up to date.
>> `
>> and
>> `Already up to date. Yeeah!
>> `
>>
>> After:
>> `Already up to date (nothing to squash).
>> `
>> and
>> `Already up to date.
>> `
>>
>> Localizations now have two easy to understand translatable strings.
>> (All localizations of the previous strings are broken.)
>>
>> Co-authored-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Josh Soref <jsoref@gmail.com>
>> ---
>
> I am not sure why this is Co-au, and not the more usual "Helped-by".
>
> The patch text makes sense to me.

Actually, not so fast.  The end-users do not care really where the
message originates.

$ git grep -e 'Already up[- ]to' \*.c
maint:builtin/merge.c:		finish_up_to_date(_("Already up to date."));
maint:builtin/merge.c:			finish_up_to_date(_("Already up to date. Yeeah!"));
maint:merge-ort-wrappers.c:		printf(_("Already up to date!"));
maint:merge-recursive.c:		output(opt, 0, _("Already up to date!"));
maint:notes-merge.c:			printf("Already up to date!\n");

It probably makes sense to replace the exclamation point with a full
stop for others, no?


Also, I didn't notice when reading the patch submission earlier, but
what does

>> (All localizations of the previous strings are broken.)

mean, exactly?  Do you mean to say

    Because this changes some messages, the old messages that were
    already translated will no longer be used, and these new
    messages need to be translated anew.

or

    Because of (...some unstated reason...), the entries in the
    message database in po/ that were meant to translate the old
    messages this patch updates were not correctly used.

or something else?

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

[Josh Soref]

"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
> Co-authored-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Josh Soref <jsoref@gmail.com>

Junio C Hamano <gitster@pobox.com> writes:
> I am not sure why this is Co-au, and not the more usual "Helped-by".

If you look at the thread, you'll see that the code in question was
written by Eric [1]. The only change from it was the addition of
`void` to the function prototype by me.

That said, it's my first foray into patches for git. I didn't use
helped-by because gitgitgadget didn't tell me to [2].

I have no opinion on the details. At this point, it's likely that a
second commit would include a helped-by referencing you. Maybe. I'm
not sure of the semantics of helped-by [2].

Junio C Hamano <gitster@pobox.com> wrote:
> Actually, not so fast.  The end-users do not care really where the
> message originates.
>
> $ git grep -e 'Already up[- ]to' \*.c
> maint:builtin/merge.c:          finish_up_to_date(_("Already up to date."));
> maint:builtin/merge.c:                  finish_up_to_date(_("Already up to date. Yeeah!"));
> maint:merge-ort-wrappers.c:             printf(_("Already up to date!"));
> maint:merge-recursive.c:                output(opt, 0, _("Already up to date!"));
> maint:notes-merge.c:                    printf("Already up to date!\n");
>
> It probably makes sense to replace the exclamation point with a full
> stop for others, no?

Maybe. I'm not sure what they mean.

I'm fully capable of wearing a grammar / UX hat and rewriting the
entire UX for a project if you invite me to do so.

I generally try not to do that when I initially approach a project, I
prefer to get more comfortable w/ it and let it get more comfortable
w/ me before I make significant change proposals.

(As an aside, I did start looking into what these messages meant /
what to change them to, but other things have come up, and I've
decided that I should at least respond to this message instead of just
appearing to disappear.)

> Also, I didn't notice when reading the patch submission earlier, but
> what does
>
> >> (All localizations of the previous strings are broken.)
>
> mean, exactly?  Do you mean to say
>
>     Because this changes some messages, the old messages that were
>     already translated will no longer be used, and these new
>     messages need to be translated anew.

Yes, this is what I meant. It's probably technically obvious to some
people, but since I'm invited to describe the effects of my change, it
seemed worth noting. Anyone cutting a release with this commit but not
updating the translations would be bleeding en-US into the
translations where before they would have had translated content.
(Whether that translated content was any good given the fact that it
was pasted together is a different story, but...)

[1] https://lore.kernel.org/git/CAPig+cT=xeLn9KNHz7hiNWo0QTfc1zZ1X-czJ4n503RRhBA0XQ@mail.gmail.com/
[2] https://github.com/gitgitgadget/gitgitgadget/issues/543

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

[Eric Sunshine]

On Sat, May 1, 2021 at 9:51 PM Josh Soref <jsoref@gmail.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > I am not sure why this is Co-au, and not the more usual "Helped-by".
>
> If you look at the thread, you'll see that the code in question was
> written by Eric [1]. The only change from it was the addition of
> `void` to the function prototype by me.

Oops, I suppose I've been doing too much Go and C++ lately and am
forgetting `void`.

I don't have a strong opinion between Co-authored-by: and Helped-by:
in this case. Here's my sign-off if you want to retain Co-authored-by:

    Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

> Junio C Hamano <gitster@pobox.com> wrote:
> > Actually, not so fast.  The end-users do not care really where the
> > message originates.
> >
> > $ git grep -e 'Already up[- ]to' \*.c
> > maint:builtin/merge.c:          finish_up_to_date(_("Already up to date."));
> > maint:builtin/merge.c:                  finish_up_to_date(_("Already up to date. Yeeah!"));
> > maint:merge-ort-wrappers.c:             printf(_("Already up to date!"));
> > maint:merge-recursive.c:                output(opt, 0, _("Already up to date!"));
> > maint:notes-merge.c:                    printf("Already up to date!\n");
> >
> > It probably makes sense to replace the exclamation point with a full
> > stop for others, no?
>
> Maybe. I'm not sure what they mean.
>
> I generally try not to do that when I initially approach a project, I
> prefer to get more comfortable w/ it and let it get more comfortable
> w/ me before I make significant change proposals.

Indeed. While it might be nice to settle upon a single punctuation
style for these messages, I don't see this as a requirement of the
patch in question. It could, of course, be re-rolled as a two-patch
series in which the second patch addresses the exclamation points, but
fixing the punctuation could also be done later as a follow-up patch
by someone (it doesn't need to be you). So, I don't see a good reason
to hold up the current patch which stands nicely on its own.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Indeed. While it might be nice to settle upon a single punctuation
> style for these messages, I don't see this as a requirement of the
> patch in question. It could, of course, be re-rolled as a two-patch
> series in which the second patch addresses the exclamation points, but
> fixing the punctuation could also be done later as a follow-up patch
> by someone (it doesn't need to be you). So, I don't see a good reason
> to hold up the current patch which stands nicely on its own.

I would agree in general, especially for a patch that fixes some
behaviour that hurts people *and* does a clean-up while at it, that
it is OK that the secondary "clean-up" part does not do as through
job as it should.

But isn't the premise of this particular patch "these 'already
up-to-date' messages puzzle readers by being sligntly different,
when the differences are not meant to convey anything, so let's
unify them and make them more coherent to help readers and
translators", is it?  I think that was why "Yeeah!" was removed, for
example.  Now we were made aware of the presence of "Already up to
date" vs "Already up to date!" by the "grep" tool.  Leaving half the
grep hits unaddressed makes the patch look like stopping halfway the
task it started to do.

So, in this particular case, I do not agree with any of the
"two-patch" or "follow-up" or "somebody else can do so".

Thanks.

[PATCH v4 0/2] normalize & fix merge "up to date" messages

[Eric Sunshine]

This is a re-roll of Josh Soref's v3[1] which fixes swapped components
of an "Already up to date" message emitted by builtin/merge. This
re-roll additionally normalizes punctuation of "Already up to date"
messages throughout the project.

I kept Josh as author of patch [2/2] but completely rewrote the commit
message to refocus it is a simple fix for a nearly 13 year old bug.

[1]: https://lore.kernel.org/git/pull.934.v3.git.1619052906768.gitgitgadget@gmail.com/

Eric Sunshine (1):
  merge(s): apply consistent punctuation to "up to date" messages

Josh Soref (1):
  merge: fix swapped "up to date" message components

 builtin/merge.c      | 14 +++++++++-----
 merge-ort-wrappers.c |  2 +-
 merge-recursive.c    |  2 +-
 notes-merge.c        |  2 +-
 4 files changed, 12 insertions(+), 8 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  3f96947e3a merge(s): apply consistent punctuation to "up to date" messages
1:  8cd2b8c335 ! 2:  5885b18b7f git-merge: rewrite already up to date message
    @@ Metadata
     Author: Josh Soref <jsoref@gmail.com>
     
      ## Commit message ##
    -    git-merge: rewrite already up to date message
    +    merge: fix swapped "up to date" message components
     
    -    Usually, it is easier to read a message if it makes its primary
    -    point first, before giving a parenthetical note.
    +    The rewrite of git-merge from shell to C in 1c7b76be7d (Build in merge,
    +    2008-07-07) accidentally transformed the message:
     
    -    Possible messages before include:
    -    ` (nothing to squash)Already up to date.
    -    `
    -    and
    -    `Already up to date. Yeeah!
    -    `
    +        Already up-to-date. (nothing to squash)
     
    -    After:
    -    `Already up to date (nothing to squash).
    -    `
    -    and
    -    `Already up to date.
    -    `
    +    to:
     
    -    Localizations now have two easy to understand translatable strings.
    -    (All localizations of the previous strings are broken.)
    +        (nothing to squash)Already up-to-date.
    +
    +    due to reversed printf() arguments. This problem has gone unnoticed
    +    despite being touched over the years by 7f87aff22c (Teach/Fix pull/fetch
    +    -q/-v options, 2008-11-15) and bacec47845 (i18n: git-merge basic
    +    messages, 2011-02-22), and tangentially by bef4830e88 (i18n: merge: mark
    +    messages for translation, 2016-06-17) and 7560f547e6 (treewide: correct
    +    several "up-to-date" to "up to date", 2017-08-23).
    +
    +    Fix it by restoring the message to its intended order. While at it, help
    +    translators out by avoiding "sentence Lego".
    +
    +    [es: rewrote commit message]
     
         Co-authored-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Josh Soref <jsoref@gmail.com>
    +    Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
     
      ## builtin/merge.c ##
     @@ builtin/merge.c: static void restore_state(const struct object_id *head,
    @@ builtin/merge.c: static void restore_state(const struct object_id *head,
     -		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
     +	if (verbosity >= 0) {
     +		if (squash)
    -+			puts(_("Already up to date (nothing to squash)."));
    ++			puts(_("Already up to date. (nothing to squash)"));
     +		else
     +			puts(_("Already up to date."));
     +	}
    @@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
      			}
      		}
      		if (up_to_date) {
    --			finish_up_to_date(_("Already up to date. Yeeah!"));
    +-			finish_up_to_date(_("Already up to date."));
     +			finish_up_to_date();
      			goto done;
      		}
-- 
2.31.1.607.g51e8a6a459

[PATCH v4 1/2] merge(s): apply consistent punctuation to "up to date" messages

[Eric Sunshine]

Although the various "Already up to date" messages resulting from merge
attempts share identical phrasing, they use a mix of punctuation ranging
from "." to "!" and even "Yeeah!", which leads to extra work for
translators. Ease the job of translators by settling upon "." as
punctuation for all such messages.

While at it, take advantage of printf_ln() to further ease the
translation task so translators need not worry about line termination,
and fix a case of missing line termination in the (unused)
merge_ort_nonrecursive() function.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/merge.c      | 2 +-
 merge-ort-wrappers.c | 2 +-
 merge-recursive.c    | 2 +-
 notes-merge.c        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 388619536a..3472a0ce3b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1610,7 +1610,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			}
 		}
 		if (up_to_date) {
-			finish_up_to_date(_("Already up to date. Yeeah!"));
+			finish_up_to_date(_("Already up to date."));
 			goto done;
 		}
 	}
diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c
index 7eec25f93a..ad04106169 100644
--- a/merge-ort-wrappers.c
+++ b/merge-ort-wrappers.c
@@ -30,7 +30,7 @@ int merge_ort_nonrecursive(struct merge_options *opt,
 		return -1;
 
 	if (oideq(&merge_base->object.oid, &merge->object.oid)) {
-		printf(_("Already up to date!"));
+		printf_ln(_("Already up to date."));
 		return 1;
 	}
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 27b222ae49..8ca5e3142d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3462,7 +3462,7 @@ static int merge_trees_internal(struct merge_options *opt,
 	}
 
 	if (oideq(&merge_base->object.oid, &merge->object.oid)) {
-		output(opt, 0, _("Already up to date!"));
+		output(opt, 0, _("Already up to date."));
 		*result = head;
 		return 1;
 	}
diff --git a/notes-merge.c b/notes-merge.c
index d2771fa3d4..321155fc87 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -628,7 +628,7 @@ int notes_merge(struct notes_merge_options *o,
 	if (oideq(&remote->object.oid, base_oid)) {
 		/* Already merged; result == local commit */
 		if (o->verbosity >= 2)
-			printf("Already up to date!\n");
+			printf_ln("Already up to date.");
 		oidcpy(result_oid, &local->object.oid);
 		goto found_result;
 	}
-- 
2.31.1.607.g51e8a6a459

[PATCH v4 2/2] merge: fix swapped "up to date" message components

[Eric Sunshine]

From: Josh Soref <jsoref@gmail.com>

The rewrite of git-merge from shell to C in 1c7b76be7d (Build in merge,
2008-07-07) accidentally transformed the message:

    Already up-to-date. (nothing to squash)

to:

    (nothing to squash)Already up-to-date.

due to reversed printf() arguments. This problem has gone unnoticed
despite being touched over the years by 7f87aff22c (Teach/Fix pull/fetch
-q/-v options, 2008-11-15) and bacec47845 (i18n: git-merge basic
messages, 2011-02-22), and tangentially by bef4830e88 (i18n: merge: mark
messages for translation, 2016-06-17) and 7560f547e6 (treewide: correct
several "up-to-date" to "up to date", 2017-08-23).

Fix it by restoring the message to its intended order. While at it, help
translators out by avoiding "sentence Lego".

[es: rewrote commit message]

Co-authored-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Josh Soref <jsoref@gmail.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/merge.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 3472a0ce3b..eddb8ae70d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -393,10 +393,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);
 }
 
@@ -1522,7 +1526,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * If head can reach all the merge then we are up to date.
 		 * but first the most common case of merging one remote.
 		 */
-		finish_up_to_date(_("Already up to date."));
+		finish_up_to_date();
 		goto done;
 	} else if (fast_forward != FF_NO && !remoteheads->next &&
 			!common->next &&
@@ -1610,7 +1614,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			}
 		}
 		if (up_to_date) {
-			finish_up_to_date(_("Already up to date."));
+			finish_up_to_date();
 			goto done;
 		}
 	}
-- 
2.31.1.607.g51e8a6a459

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, May 1, 2021 at 9:51 PM Josh Soref <jsoref@gmail.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> > I am not sure why this is Co-au, and not the more usual "Helped-by".
>>
>> If you look at the thread, you'll see that the code in question was
>> written by Eric [1]. The only change from it was the addition of
>> `void` to the function prototype by me.
>
> Oops, I suppose I've been doing too much Go and C++ lately and am
> forgetting `void`.
>
> I don't have a strong opinion between Co-authored-by: and Helped-by:
> in this case. Here's my sign-off if you want to retain Co-authored-by:
>
>     Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

I am not in principle opposed to the idea of co-authored-by; for
this particular one, we historically have used Helped-by (i.e. a
reviewer offers "writing it this way is cleaner" suggestions on the
list and then gets credited on the next version), and it wasn't
clear to me if you consented to be a co-author of the patch.  If the
party who were named as a co-author responded that it is OK, I would
be perfectly fine.

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

[Eric Sunshine]

On Sun, May 2, 2021 at 2:26 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > I don't have a strong opinion between Co-authored-by: and Helped-by:
> > in this case. Here's my sign-off if you want to retain Co-authored-by:
> >
> >     Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>
> I am not in principle opposed to the idea of co-authored-by; for
> this particular one, we historically have used Helped-by (i.e. a
> reviewer offers "writing it this way is cleaner" suggestions on the
> list and then gets credited on the next version), and it wasn't
> clear to me if you consented to be a co-author of the patch.  If the
> party who were named as a co-author responded that it is OK, I would
> be perfectly fine.

It wasn't my intention to be co-author but I'm OK with the designation
in this particular case since I did end up authoring all the code in
the patch (aside from `void`), even if that authorship was by accident
through the circumstance of reviewing the patch (but, as mentioned
above, I can go either way with it).

Re: [PATCH v4 2/2] merge: fix swapped "up to date" message components

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> +	if (verbosity >= 0) {
> +		if (squash)
> +			puts(_("Already up to date. (nothing to squash)"));

The original scripted Porcelain may have said so, but the placement
of full-stop in the above feels a bit strange.  Should we rephrase
it to

	Already up to date (nothing to squash).

as we are fixing the phrasing now?

Re: [PATCH v4 2/2] merge: fix swapped "up to date" message components

[Eric Sunshine]

On Mon, May 3, 2021 at 1:21 AM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > +     if (verbosity >= 0) {
> > +             if (squash)
> > +                     puts(_("Already up to date. (nothing to squash)"));
>
> The original scripted Porcelain may have said so, but the placement
> of full-stop in the above feels a bit strange.  Should we rephrase
> it to
>
>         Already up to date (nothing to squash).
>
> as we are fixing the phrasing now?

I don't have a strong opinion about it, and can go either way with it.
Josh's patch did place the full-stop after the closing parenthesis. I
can re-roll if people think that would be preferable (unless you want
to change it locally while queuing).

Re: [PATCH v4 2/2] merge: fix swapped "up to date" message components

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, May 3, 2021 at 1:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > +     if (verbosity >= 0) {
>> > +             if (squash)
>> > +                     puts(_("Already up to date. (nothing to squash)"));
>>
>> The original scripted Porcelain may have said so, but the placement
>> of full-stop in the above feels a bit strange.  Should we rephrase
>> it to
>>
>>         Already up to date (nothing to squash).
>>
>> as we are fixing the phrasing now?
>
> I don't have a strong opinion about it, and can go either way with it.
> Josh's patch did place the full-stop after the closing parenthesis. I
> can re-roll if people think that would be preferable (unless you want
> to change it locally while queuing).

I am fine to leave this outisde the topic.