Patchwork [STABLE] bundle2: localize "remote: "

login
register
mail settings
Submitter Gregory Szorc
Date Aug. 30, 2016, 8:33 p.m.
Message ID <f7ec2b2f002750a7748d.1472589213@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/16495/
State Accepted
Headers show

Comments

Gregory Szorc - Aug. 30, 2016, 8:33 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1472588883 25200
#      Tue Aug 30 13:28:03 2016 -0700
# Branch stable
# Node ID f7ec2b2f002750a7748dc5d0375035e181d1c3b0
# Parent  9a9629b9416c262ff62bbd17568fcd2a2e09b4be
bundle2: localize "remote: "

We localize "remote: " everywhere else. This unlocalized use was
introduced in b7435117d951.

This issue was discovered by Akihiko Odaki and reported at
https://bugzilla.mozilla.org/show_bug.cgi?id=1298651.
Augie Fackler - Aug. 30, 2016, 9:25 p.m.
On Tue, Aug 30, 2016 at 01:33:33PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1472588883 25200
> #      Tue Aug 30 13:28:03 2016 -0700
> # Branch stable
> # Node ID f7ec2b2f002750a7748dc5d0375035e181d1c3b0
> # Parent  9a9629b9416c262ff62bbd17568fcd2a2e09b4be
> bundle2: localize "remote: "

Fair enough, queued.

>
> We localize "remote: " everywhere else. This unlocalized use was
> introduced in b7435117d951.
>
> This issue was discovered by Akihiko Odaki and reported at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1298651.
>
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1466,17 +1466,17 @@ def handlecheckheads(op, inpart):
>      if sorted(heads) != sorted(op.repo.heads()):
>          raise error.PushRaced('repository changed while pushing - '
>                                'please try again')
>
>  @parthandler('output')
>  def handleoutput(op, inpart):
>      """forward output captured on the server to the client"""
>      for line in inpart.read().splitlines():
> -        op.ui.status(('remote: %s\n' % line))
> +        op.ui.status('%s%s\n' % (_('remote: '), line))
>
>  @parthandler('replycaps')
>  def handlereplycaps(op, inpart):
>      """Notify that a reply bundle should be created
>
>      The payload contains the capabilities information for the reply"""
>      caps = decodecaps(inpart.read())
>      if op.reply is None:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Matt Mackall - Aug. 30, 2016, 9:44 p.m.
On Tue, 2016-08-30 at 13:33 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1472588883 25200
> #      Tue Aug 30 13:28:03 2016 -0700
> # Branch stable
> # Node ID f7ec2b2f002750a7748dc5d0375035e181d1c3b0
> # Parent  9a9629b9416c262ff62bbd17568fcd2a2e09b4be
> bundle2: localize "remote: "
> 
> We localize "remote: " everywhere else. This unlocalized use was
> introduced in b7435117d951.

Ok, this is slightly more informative than Akihiko's patch.. but I'm still not
happy:

> -        op.ui.status(('remote: %s\n' % line))

See those extra parens? They were put there to defeat check-code, which reminds
you to use _() in the first place. That is a signal to me that there was intent
for this to NOT be translated.

If you don't even mention the parens, I have to assume you didn't notice them
and didn't consider the possibility that there was a reason for them.

> +        op.ui.status('%s%s\n' % (_('remote: '), line))

I'm not sure why you're doing the %s%s thing. It's more complex than necessary
and the existing string literal is already translated elsewhere.

-- 
Mathematics is the supreme nostalgia of our time.
Augie Fackler - Aug. 30, 2016, 9:51 p.m.
On Tue, Aug 30, 2016 at 5:44 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Tue, 2016-08-30 at 13:33 -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1472588883 25200
>> #      Tue Aug 30 13:28:03 2016 -0700
>> # Branch stable
>> # Node ID f7ec2b2f002750a7748dc5d0375035e181d1c3b0
>> # Parent  9a9629b9416c262ff62bbd17568fcd2a2e09b4be
>> bundle2: localize "remote: "
>>
>> We localize "remote: " everywhere else. This unlocalized use was
>> introduced in b7435117d951.

(I've dropped the patch from my queue for now.)

> Ok, this is slightly more informative than Akihiko's patch.. but I'm still not
> happy:
>
>> -        op.ui.status(('remote: %s\n' % line))
>
> See those extra parens? They were put there to defeat check-code, which reminds
> you to use _() in the first place. That is a signal to me that there was intent
> for this to NOT be translated.
>
> If you don't even mention the parens, I have to assume you didn't notice them
> and didn't consider the possibility that there was a reason for them.
>
>> +        op.ui.status('%s%s\n' % (_('remote: '), line))
>
> I'm not sure why you're doing the %s%s thing. It's more complex than necessary
> and the existing string literal is already translated elsewhere.
>
> --
> Mathematics is the supreme nostalgia of our time.
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Gregory Szorc - Aug. 31, 2016, 12:49 a.m.
On Tue, Aug 30, 2016 at 2:44 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Tue, 2016-08-30 at 13:33 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1472588883 25200
> > #      Tue Aug 30 13:28:03 2016 -0700
> > # Branch stable
> > # Node ID f7ec2b2f002750a7748dc5d0375035e181d1c3b0
> > # Parent  9a9629b9416c262ff62bbd17568fcd2a2e09b4be
> > bundle2: localize "remote: "
> >
> > We localize "remote: " everywhere else. This unlocalized use was
> > introduced in b7435117d951.
>
> Ok, this is slightly more informative than Akihiko's patch.. but I'm still
> not
> happy:
>
> > -        op.ui.status(('remote: %s\n' % line))
>
> See those extra parens? They were put there to defeat check-code, which
> reminds
> you to use _() in the first place. That is a signal to me that there was
> intent
> for this to NOT be translated.
>
> If you don't even mention the parens, I have to assume you didn't notice
> them
> and didn't consider the possibility that there was a reason for them.
>

You are correct about me not recognizing that the parens were there to
defeat check-code.

However, I'm going to posit that _() not being used was accidental. We
translate "remote: " everywhere else. This code is running locally (not on
the server). So I don't see why we wouldn't translate.


>
> > +        op.ui.status('%s%s\n' % (_('remote: '), line))
>
> I'm not sure why you're doing the %s%s thing. It's more complex than
> necessary
> and the existing string literal is already translated elsewhere.
>

That's fair.
Matt Mackall - Aug. 31, 2016, 6:58 a.m.
On Tue, 2016-08-30 at 17:49 -0700, Gregory Szorc wrote:

> However, I'm going to posit that _() not being used was accidental. We
> translate "remote: " everywhere else. This code is running locally (not on
> the server). So I don't see why we wouldn't tranhslate.

I agree it's almost certainly wrong.

But the thing I'm getting at is:

- we've had rules forever to make sure all write/status messages get translated
- so it's weird when one isn't
- and extra weird when an obvious one like this has willfully defeated the rule
- such weirdness should trigger a higher level of investigation and rationale 

When we fix things that are obviously wrong, we always risk breaking things that
are actually subtly right. A really good warning sign that we're about to do
that is when we find something that's obviously wrong.. but also inexplicable.

-- 
Mathematics is the supreme nostalgia of our time.

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1466,17 +1466,17 @@  def handlecheckheads(op, inpart):
     if sorted(heads) != sorted(op.repo.heads()):
         raise error.PushRaced('repository changed while pushing - '
                               'please try again')
 
 @parthandler('output')
 def handleoutput(op, inpart):
     """forward output captured on the server to the client"""
     for line in inpart.read().splitlines():
-        op.ui.status(('remote: %s\n' % line))
+        op.ui.status('%s%s\n' % (_('remote: '), line))
 
 @parthandler('replycaps')
 def handlereplycaps(op, inpart):
     """Notify that a reply bundle should be created
 
     The payload contains the capabilities information for the reply"""
     caps = decodecaps(inpart.read())
     if op.reply is None: