git-merge: move space to between strings
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
@@ -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
Quoted 8 lines · Josh Soref via GitGitGadget
From: Josh Soref <[email protected]>
Signed-off-by Josh Soref <[email protected]>---
git-merge: move space to between stringsGitHub 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.
PATCH v2 1/1 git-merge: move primary point before parenthetical
In reply to Josh Soref via GitGitGadgetFrom: 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
@@ -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
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
Signed-off-by Josh Soref <[email protected]>---
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();
PATCH v3 1/1 git-merge: rewrite already up to date message
In reply to Josh Soref via GitGitGadgetFrom: 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
@@ -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
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.
[...]Signed-off-by Josh Soref <[email protected]>---
Changes since v2:*
finish_up_to_datenow 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.
Re: [PATCH v3] git-merge: rewrite already up to date message
In reply to Josh Soref via GitGitGadgetQuoted 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.)
Co-authored-by Eric Sunshine <[email protected]>Signed-off-by Josh Soref <[email protected]>---
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@@ -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
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
Co-authored-by Eric Sunshine <[email protected]>Signed-off-by Josh Soref <[email protected]>---
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?
Re: [PATCH v3] git-merge: rewrite already up to date message
Quoted 3 lines · Josh Soref via GitGitGadget
From: Josh Soref <[email protected]>
Co-authored-by Eric Sunshine <[email protected]>Signed-off-by 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
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
voidto 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.
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.
v4 cover normalize & fix merge "up to date" messages
In reply to Josh Soref via GitGitGadgetThis 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
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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v4 2/2 merge: fix swapped "up to date" message components
In reply to Eric SunshineFrom: 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
@@ -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
Re: [PATCH v3] git-merge: rewrite already up to date message
In reply to Eric SunshineQuoted 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
voidto 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:Signed-off-by Eric Sunshine <[email protected]>
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
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:Signed-off-by Eric Sunshine <[email protected]>
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).
Re: [PATCH v4 2/2] merge: fix swapped "up to date" message components
In reply to Eric Sunshinebuiltin/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?
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).
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.