Patchwork [2,of,2,V2] subrepo: make the 'in subrepo' error string easier to find by external tools

login
register
mail settings
Submitter Angel Ezquerra
Date Jan. 8, 2013, 10:43 p.m.
Message ID <289047302d898a440142.1357684984@Angel-PC.localdomain>
Download mbox | patch
Permalink /patch/510/
State Changes Requested
Delegated to: Bryan O'Sullivan
Headers show

Comments

Angel Ezquerra - Jan. 8, 2013, 10:43 p.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1357231345 -3600
# Node ID 289047302d898a4401424ac05f1e8ec3d9343945
# Parent  5a1a5555319c13de73e51d271de5356b4a0656d0
subrepo: make the 'in subrepo' error string easier to find by external tools

This patch is meant to make it easier for tools that wrap the mercurial
output (such as TortoiseHg) to find the "in subrepo" string that (since
9e3910db4e78) is appended after subrepo error messages, particularly when the
mercurial output is translated to a non-English language.

The message remains the same but the '%s' that was used to prepend the original
error message in front of the 'in subrepo' string has been moved out of the
translatable string.
Bryan O'Sullivan - Jan. 9, 2013, 12:08 a.m.
On Tue, Jan 08, 2013 at 11:43:04PM +0100, Angel Ezquerra wrote:
> subrepo: make the 'in subrepo' error string easier to find by external tools

I don't understand how this patch makes that easier. Could you explain, please?
Angel Ezquerra - Jan. 9, 2013, 7:59 a.m.
On Wed, Jan 9, 2013 at 1:08 AM, Bryan O'Sullivan <bos@serpentine.com> wrote:
> On Tue, Jan 08, 2013 at 11:43:04PM +0100, Angel Ezquerra wrote:
>> subrepo: make the 'in subrepo' error string easier to find by external tools
>
> I don't understand how this patch makes that easier. Could you explain, please?

As I said on my initial patch series "0 message" this is in no way
essential, but it would be nice.

First of all, the original code is slightly wrong because it contains
two %s fields which a translator could potentially swap with the
mistaken impression that the corresponding text would also be swapped,
which is not the case. That can be fixed by using a dictionary with
explicit string interpolation references, but that would not help me
with the real problem.

The actual reason for this patch is perhaps a bit hard to explain
without an example and some context.

In TortortoiseHg we show an "infobar" when an error happens. In the
past these error messages were confusing when they showed subrepo
error messages because they gave no context (it was not possible to
tell that the error came from a subrepo).

Now mercurial adds an "(in subrepo %s)" "tag" after those messages,
which is much better. To improve things futher I am trying to convert
the repository paths (the "%s" after "in subrepo") into links, that,
when clicked, open the corresponding subrepo at the corresponding
revision (if a revision id is also shown on the error message). For
example the message:

abort: push creates new remote head 2dbf46327043! (in subrepo s)

must be "linkified" by becoming (internally):

abort: push creates new remote head <a
href="s?2dbf46327043">2dbf46327043</a>! (in subrepo <a
href="s?2dbf46327043">s</a>)

In order to do so we currently use a regular expression that looks for
"(in subrepo .+)$" (or something similar). Now that the
SubrepoAbortException has a "subrepo" property this can be made even
more explicit by looking for the actual subrepo path.

However, while this works fine when mercurial's output is in English,
it does not work for other languages.. To cope with non English
messages we have a couple of alternatives (I think).

1. We can ask our translators to translate the "(in subrepo %s)" bit
exactly as mercurial's translators
2. We can get mercurial's translated "(in subrepo %s)" string.

In both cases we must get that translated string and convert it into
our regular expression.

#1 is not a very good solution because we depend on (potentially
different) translators being consistent.

#2 is better (I think) but the fact that the current translated string
is "%s (in subrepo %s)" makes using it a bit hard. We must either
remove the initial "%s", which could potentially not be the only thing
in the translated string (e.g. "%s (in subrepo %s)" could potentially
become "subrepo error %s encontrado en el subrepositorio %s"), where
%s is an unknown error message; or we could take it into account in
our the regular expression search (e.g. ".+ (in subrepo MY_SUBREPO)",
but it could potentially fail if the error message contained the "in
subrepo MY_SUBREPO" text in it (which shouldn't happen, but still is a
bit of a concern).

By making sure that the actual, error message itself is the only and
first thing in the translated error message that TortoiseHg gets, we
can just get that translated mercurial string, interpolate it using
the subrepo path that we get from the exception, and look for it.

That being said, maybe there is a better, alternative approach to cope
with this problem?

As I said, not an essential thing, but it would be simpler on the
TortoiseHg side IMO, and it also would fix an existing problem.

Cheers,

Angel
Bryan O'Sullivan - Jan. 9, 2013, 5:13 p.m.
On Tue, Jan 8, 2013 at 11:59 PM, Angel Ezquerra <angel.ezquerra@gmail.com>wrote:

>
> As I said on my initial patch series "0 message" this is in no way
> essential, but it would be nice.
>

Remember, it's rare that people read the "0 of N" message (myself included).

If there's information relevant to understanding a patch, it belongs in the
patch itself.


> The actual reason for this patch is perhaps a bit hard to explain
> without an example and some context.
>

Thanks for the explanation. Could you trim it down to about a paragraph and
include it in a resend of this patch, please?

Patch

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -29,7 +29,7 @@ 
             raise ex
         except error.Abort, ex:
             subrepo = subrelpath(self)
-            errormsg = _('%s (in subrepo %s)') % (str(ex), subrepo)
+            errormsg = str(ex) + ' ' + _('(in subrepo %s)') % subrepo
             # avoid handling this exception by raising a SubrepoAbort exception
             raise SubrepoAbort(errormsg, hint=ex.hint, subrepo=subrepo)
         return res