Skip to contentSkip to conversation

git-merge: move space to between strings

PATCH In review Josh Soref via GitGitGadget 4 participants 19 messages Apr 18, 2021 – May 3, 2021

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 git-merge: move space to between strings

From: Josh Soref <[email protected]>

---
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(-)
builtin/merge.c +1 -1
index 062e91144125..0d8c782cccb2 100644
@@ -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

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 8 lines · Josh Soref via GitGitGadget

From: Josh Soref <[email protected]>

---
git-merge: move space to between strings

GitHub Actions show things like:

Quoted 8 lines

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

#2 lore raw

PATCH v2 1/1 git-merge: move primary point before parenthetical

In reply to Josh Soref via GitGitGadget

From: Josh Soref <[email protected]>

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

---
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 <[email protected]>

## 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).
+ `

@@ 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(-)
builtin/merge.c +3 -3
index 062e91144125..aad180010670 100644
@@ -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

Signature · 1 line
gitgitgadget
#3 lore raw

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

Quoted 10 lines · Josh Soref via GitGitGadget

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

Quoted 15 lines

---

builtin/merge.c +2 -2
@@ -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"));

builtin/merge.c int cmd_merge(int argc, const char **argv, const char *prefix) · line 1566

Quoted 2 lines
builtin/merge.c +1 -1
@@ -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();
#4 lore raw

PATCH v3 1/1 git-merge: rewrite already up to date message

In reply to Josh Soref via GitGitGadget

From: Josh Soref <[email protected]>

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

---
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 <[email protected]>
      
       ## 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 <[email protected]>
          Signed-off-by: Josh Soref <[email protected]>
      
       ## 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(-)
builtin/merge.c +9 -5
index 062e91144125..f8c3d0687eaf 100644
@@ -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

Signature · 1 line
gitgitgadget
#5 lore raw

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

Quoted 10 lines · Josh Soref via GitGitGadget

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

---
Changes since v2:

* finish_up_to_date now figures out the answer on its own to address feedback from Eric Sunshine

Quoted 6 lines

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

Quoted 20 lines
builtin/merge.c +8 -4
@@ -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();

builtin/merge.c int cmd_merge(int argc, const char **argv, const char *prefix) · line 1570

Quoted 2 lines
builtin/merge.c +1 -1
@@ -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.

#6 lore raw

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

In reply to Josh Soref via GitGitGadget
Quoted 19 lines · Josh Soref via GitGitGadget

From: Josh Soref <[email protected]>

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

Quoted 6 lines

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

---

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

The patch text makes sense to me.

Thanks.

Quoted 42 lines
builtin/merge.c +9 -5
index 062e91144125..f8c3d0687eaf 100644
@@ -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

#7 lore raw

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

Quoted 24 lines · Junio C Hamano
Quoted 22 lines · Josh Soref via GitGitGadget

From: Josh Soref <[email protected]>

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

Quoted 7 lines
Quoted 3 lines

---

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

Quoted 1 line
Quoted 1 line

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

#8 lore raw

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

Quoted 3 lines · Josh Soref via GitGitGadget

From: Josh Soref <[email protected]>

Quoted 1 line · Junio C Hamano

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

Quoted 10 lines · Junio C Hamano

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");
Quoted 2 lines

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

Quoted 10 lines

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

Quoted 1 line
Quoted 1 line

(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

#9 lore raw

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

Quoted 6 lines · Josh Soref
Quoted 1 line · Junio C Hamano

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:

Quoted 11 lines
Quoted 10 lines · Junio C Hamano

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");
Quoted 8 lines
Quoted 2 lines

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.

#10 lore raw

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

Quoted 7 lines · Eric Sunshine

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.

#13 lore raw

v4 cover normalize & fix merge "up to date" messages

In reply to Josh Soref via GitGitGadget

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/[email protected]/

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 <[email protected]>

## 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]

+ Signed-off-by: Eric Sunshine <[email protected]>

## 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;
}

Signature · 1 line
2.31.1.607.g51e8a6a459
#14 lore raw

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

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.

---
 builtin/merge.c      | 2 +-
 merge-ort-wrappers.c | 2 +-
 merge-recursive.c    | 2 +-
 notes-merge.c        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
builtin/merge.c +1 -1
index 388619536a..3472a0ce3b 100644
@@ -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;
 		}
 	}
merge-ort-wrappers.c +1 -1
index 7eec25f93a..ad04106169 100644
@@ -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;
 	}
 
merge-recursive.c +1 -1
index 27b222ae49..8ca5e3142d 100644
@@ -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;
 	}
notes-merge.c +1 -1
index d2771fa3d4..321155fc87 100644
@@ -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;
 	}
Signature · 1 line
2.31.1.607.g51e8a6a459
#15 lore raw

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

In reply to Eric Sunshine

From: Josh Soref <[email protected]>

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]

---
 builtin/merge.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
builtin/merge.c +9 -5
index 3472a0ce3b..eddb8ae70d 100644
@@ -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;
 		}
 	}
Signature · 1 line
2.31.1.607.g51e8a6a459
#11 lore raw

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

In reply to Eric Sunshine
Quoted 8 lines · Eric Sunshine
Quoted 6 lines · Josh Soref
Quoted 1 line · Junio C Hamano

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.

Quoted 7 lines

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:

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.

#12 lore raw

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

Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Eric Sunshine

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:

Quoted 7 lines

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

#16 lore raw

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

In reply to Eric Sunshine

builtin/merge.c static void restore_state(const struct object_id *head, · line 400

Quoted 3 lines · Eric Sunshine
builtin/merge.c +3 -0
+	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?

#17 lore raw

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

Quoted 5 lines · Junio C Hamano

builtin/merge.c static void restore_state(const struct object_id *head, · line 400

Quoted 3 lines · Eric Sunshine
builtin/merge.c +3 -0
+     if (verbosity >= 0) {
+             if (squash)
+                     puts(_("Already up to date. (nothing to squash)"));
Quoted 7 lines

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

#18 lore raw

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

Quoted 10 lines · Eric Sunshine
Quoted 9 lines · Junio C Hamano

builtin/merge.c static void restore_state(const struct object_id *head, · line 400

Quoted 3 lines · Eric Sunshine
builtin/merge.c +3 -0
+     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

Quoted 8 lines
Quoted 3 lines

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.