diff mbox series

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

Message ID pull.934.v3.git.1619052906768.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series [v3] git-merge: rewrite already up to date message | expand

Commit Message

Josh Soref April 22, 2021, 12:55 a.m. UTC
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(-)


base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0

Comments

Eric Sunshine April 22, 2021, 3:41 a.m. UTC | #1
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.
Junio C Hamano April 28, 2021, 4:04 a.m. UTC | #2
"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
Junio C Hamano April 29, 2021, 7:52 a.m. UTC | #3
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?
Josh Soref May 2, 2021, 1:51 a.m. UTC | #4
"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
Eric Sunshine May 2, 2021, 2:15 a.m. UTC | #5
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.
Junio C Hamano May 2, 2021, 2:39 a.m. UTC | #6
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.
Junio C Hamano May 2, 2021, 6:26 a.m. UTC | #7
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.
Eric Sunshine May 2, 2021, 7:14 a.m. UTC | #8
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).
diff mbox series

Patch

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