Re: [PATCHv1 1/3] git-p4: raise exceptions from p4CmdList based on error from p4 server

From: Luke Diamand <luke@diamand.org>
Date: Jun 5, 2018, 10:56 UTC
Message-ID: <CAE5ih7-1StB8RNrobO+hpG2QOSdoscUNfVP9+muZV0_b+m+XSw@mail.gmail.com>
In-Reply-To: <CAPig+cQTTY8wc4d=4jV8GxHDOjF7xk1vjA62JS6s4Zr0uXiegg@mail.gmail.com>

On 5 June 2018 at 10:54, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 31 quoted lines

> On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand <luke@diamand.org> wrote:
>> This change lays some groundwork for better handling of rowcount errors
>> from the server, where it fails to send us results because we requested
>> too many.
>>
>> It adds an option to p4CmdList() to return errors as a Python exception.
>>
>> The exceptions are derived from P4Exception (something went wrong),
>> P4ServerException (the server sent us an error code) and
>> P4RequestSizeException (we requested too many rows/results from the
>> server database).
>>
>> This makes makes the code that handles the errors a bit easier.
>>
>> The default behavior is unchanged; the new code is enabled with a flag.
>>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>> diff --git a/git-p4.py b/git-p4.py
>> @@ -566,10 +566,30 @@ def isModeExec(mode):
>> +class P4ServerException(Exception):
>> +    """ Base class for exceptions where we get some kind of marshalled up result from the server """
>> +    def __init__(self, exit_code, p4_result):
>> +        super(P4ServerException, self).__init__(exit_code)
>> +        self.p4_result = p4_result
>> +        self.code = p4_result[0]['code']
>> +        self.data = p4_result[0]['data']
>
> The subsequent patches never seem to access any of these fields, so
> it's difficult to judge whether it's worthwhile having 'code' and
> 'data' bits split out like this.

These changes don't use it, but I thought that future changes might need them, and perhaps when I put that code in I was thinking I might need it myself.

Show 15 quoted lines

>
>> @@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
>>      if exitCode != 0:
>> -        entry = {}
>> -        entry["p4ExitCode"] = exitCode
>> -        result.append(entry)
>> +        if errors_as_exceptions:
>> +            if len(result) > 0:
>> +                data = result[0].get('data')
>> +                if data:
>> +                    m = re.search('Too many rows scanned \(over (\d+)\)', data)
>> +                    if not m:
>> +                        m = re.search('Request too large \(over (\d+)\)', data)
>
> Does 'p4' localize these error messages?

That's a good question.

The marshalled-up error from Perforce looks like this:

     ([{'generic': 35, 'code': 'error', 'data': "Too many rows scanned
(over 40); see 'p4 help maxscanrows'.\n", 'severity': 3}])

It turns out that Perforce open-sourced the P4 client in 2014 (I only recently found this out) so we can actually look at the code now!

    https://swarm.workshop.perforce.com/projects/perforce_software-p4

Clone it like this:

mkdir p4 && (cd p4 && git init && git config --add git-p4.branchList p4/2018-1:2018-1) && P4USER=guest P4PORT=workshop.perforce.com:1666 git p4 clone --detect-branches --destination p4 //guest/perforce_software/p4@all

Here's the code:

    // ErrorId graveyard: retired/deprecated ErrorIds.

    ErrorId MsgDb::MaxResults              = { ErrorOf( ES_DB, 32,
E_FAILED, EV_ADMIN, 1 ), "Request too large (over %maxResults%); see
'p4 help maxresults'." } ;//NOTRANS
    ErrorId MsgDb::MaxScanRows             = { ErrorOf( ES_DB, 61,
E_FAILED, EV_ADMIN, 1 ), "Too many rows scanned (over %maxScanRows%);
see 'p4 help maxscanrows'." } ;//NOTRANS

I don't think there's actually a way to make it return any language other than English though. There's a P4LANGUAGE environment variable, but it just says "this is for system integrators":

https://www.perforce.com/perforce/r15.2/manuals/cmdref/P4LANGUAGE.html

So I think probably the language is always English.

Luke