| Message ID | pull.934.v3.git.1619052906768.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] git-merge: rewrite already up to date message | expand |
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.
"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 <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 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
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.
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.
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.
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 --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; } }