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

2018-06-08 Thread Luke Diamand
>>
>>> 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. [...]
>>> So I think probably the language is always English.
>>
>> The "NOTRANS" annotation on the error messages is reassuring.
>
> I'll check it works OK on Windows; charset translation might cause a problem.

Works OK on Windows with 2017.2 client/server.


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

2018-06-05 Thread Luke Diamand
On 5 June 2018 at 20:41, Eric Sunshine  wrote:
> On Tue, Jun 5, 2018 at 6:56 AM Luke Diamand  wrote:
>> On 5 June 2018 at 10:54, Eric Sunshine  wrote:
>> > On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  wrote:
>> >> +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.
>>
>> 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!
>>
>> Here's the code:
>>
>> // ErrorId graveyard: retired/deprecated ErrorIds.
>
> Hmm, the "too many rows" error you're seeing is retired/deprecated(?).

There's some code elsewhere that suggests it's being kept alive:

// Retired ErrorIds. We need to keep these so that clients
// built with newer apis can commnunicate with older servers
// still sending these.

static ErrorId MaxResults; // DEPRECATED
static ErrorId MaxScanRows; // DEPRECATED


>
>> 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. [...]
>> So I think probably the language is always English.
>
> The "NOTRANS" annotation on the error messages is reassuring.

I'll check it works OK on Windows; charset translation might cause a problem.


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

2018-06-05 Thread Eric Sunshine
On Tue, Jun 5, 2018 at 6:56 AM Luke Diamand  wrote:
> On 5 June 2018 at 10:54, Eric Sunshine  wrote:
> > On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  wrote:
> >> +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.
>
> 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!
>
> Here's the code:
>
> // ErrorId graveyard: retired/deprecated ErrorIds.

Hmm, the "too many rows" error you're seeing is retired/deprecated(?).

> 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. [...]
> So I think probably the language is always English.

The "NOTRANS" annotation on the error messages is reassuring.


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

2018-06-05 Thread Luke Diamand
On 5 June 2018 at 10:54, Eric Sunshine  wrote:
> On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  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 
>> ---
>> 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.

>
>> @@ -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


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

2018-06-05 Thread Eric Sunshine
On Tue, Jun 5, 2018 at 5:14 AM Luke Diamand  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 
> ---
> 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.

> @@ -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?

> +if m:
> +limit = int(m.group(1))
> +raise P4RequestSizeException(exitCode, result, limit)
> +
> +raise P4ServerException(exitCode, result)
> +else:
> +raise P4Exception(exitCode)
> +else:
> +entry = {}
> +entry["p4ExitCode"] = exitCode
> +result.append(entry)


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

2018-06-05 Thread Luke Diamand
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 
---
 git-p4.py | 44 
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 66652f8280..d856078ec8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,10 +566,30 @@ def isModeExec(mode):
 # otherwise False.
 return mode[-3:] == "755"
 
+class P4Exception(Exception):
+""" Base class for exceptions from the p4 client """
+def __init__(self, exit_code):
+self.p4ExitCode = exit_code
+
+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']
+
+class P4RequestSizeException(P4ServerException):
+""" One of the maxresults or maxscanrows errors """
+def __init__(self, exit_code, p4_result, limit):
+super(P4RequestSizeException, self).__init__(exit_code, p4_result)
+self.limit = limit
+
 def isModeExecChanged(src_mode, dst_mode):
 return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False):
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
+errors_as_exceptions=False):
 
 if isinstance(cmd,basestring):
 cmd = "-G " + cmd
@@ -616,9 +636,25 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, 
skip_info=False):
 pass
 exitCode = p4.wait()
 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)
+
+if m:
+limit = int(m.group(1))
+raise P4RequestSizeException(exitCode, result, limit)
+
+raise P4ServerException(exitCode, result)
+else:
+raise P4Exception(exitCode)
+else:
+entry = {}
+entry["p4ExitCode"] = exitCode
+result.append(entry)
 
 return result
 
-- 
2.17.0.392.gdeb1a6e9b7