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