[PATCHv1 0/1] git-p4: better error reporting

[PATCHv1 0/1] git-p4: better error reporting

[Luke Diamand]

If git-p4 cannot talk to the Perforce server, it will either give a
confusing error message, or just crash. Even I get tripped up by this.

This change just checks that the client can talk to the server, and in
particular that the user is logged in (the default login timeout is 12
hours).

It would be nice to also check for ticket expiration during long
Perforce operations, but this change does not attempt to do that.

Luke Diamand (1):
  git-p4: better error reporting when p4 fails

 git-p4.py         | 55 +++++++++++++++++++++++++++++++++
 t/t9833-errors.sh | 78 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)
 create mode 100755 t/t9833-errors.sh

-- 
2.17.0.392.gdeb1a6e9b7

[PATCHv1 1/1] git-p4: better error reporting when p4 fails

[Luke Diamand]

Currently when p4 fails to run, git-p4 just crashes with an obscure
error message.

For example, if the P4 ticket has expired, you get:

  Error: Cannot locate perforce checkout of <path> in client view

This change checks whether git-p4 can talk to the Perforce server when
the first P4 operation is attempted, and tries to print a meaningful
error message if it fails.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py         | 55 +++++++++++++++++++++++++++++++++
 t/t9833-errors.sh | 78 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)
 create mode 100755 t/t9833-errors.sh

diff --git a/git-p4.py b/git-p4.py
index b9e79f1d8b..66652f8280 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -50,6 +50,8 @@ def __str__(self):
 # Grab changes in blocks of this many revisions, unless otherwise requested
 defaultBlockSize = 512
 
+p4_access_checked = False
+
 def p4_build_cmd(cmd):
     """Build a suitable p4 command line.
 
@@ -91,6 +93,13 @@ def p4_build_cmd(cmd):
         real_cmd = ' '.join(real_cmd) + ' ' + cmd
     else:
         real_cmd += cmd
+
+    # now check that we can actually talk to the server
+    global p4_access_checked
+    if not p4_access_checked:
+        p4_access_checked = True
+        p4_check_access()
+
     return real_cmd
 
 def git_dir(path):
@@ -264,6 +273,52 @@ def p4_system(cmd):
     if retcode:
         raise CalledProcessError(retcode, real_cmd)
 
+def die_bad_access(s):
+    die("failure accessing depot: {0}".format(s.rstrip()))
+
+def p4_check_access(min_expiration=1):
+    """ Check if we can access Perforce - account still logged in
+    """
+    results = p4CmdList(["login", "-s"])
+
+    if len(results) == 0:
+        # should never get here: always get either some results, or a p4ExitCode
+        assert("could not parse response from perforce")
+
+    result = results[0]
+
+    if 'p4ExitCode' in result:
+        # p4 returned non-zero status, e.g. P4PORT invalid, or p4 not in path
+        die_bad_access("could not run p4")
+
+    code = result.get("code")
+    if not code:
+        # we get here if we couldn't connect and there was nothing to unmarshal
+        die_bad_access("could not connect")
+
+    elif code == "stat":
+        expiry = result.get("TicketExpiration")
+        if expiry:
+            expiry = int(expiry)
+            if expiry > min_expiration:
+                # ok to carry on
+                return
+            else:
+                die_bad_access("perforce ticket expires in {0} seconds".format(expiry))
+
+        else:
+            # account without a timeout - all ok
+            return
+
+    elif code == "error":
+        data = result.get("data")
+        if data:
+            die_bad_access("p4 error: {0}".format(data))
+        else:
+            die_bad_access("unknown error")
+    else:
+        die_bad_access("unknown error code {0}".format(code))
+
 _p4_version_string = None
 def p4_version_string():
     """Read the version string, showing just the last line, which
diff --git a/t/t9833-errors.sh b/t/t9833-errors.sh
new file mode 100755
index 0000000000..9ba892de7a
--- /dev/null
+++ b/t/t9833-errors.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='git p4 errors'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+test_expect_success 'add p4 files' '
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "file1"
+	)
+'
+
+# after this test, the default user requires a password
+test_expect_success 'error handling' '
+	git p4 clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		P4PORT=: test_must_fail git p4 submit 2>errmsg
+	) &&
+	p4 passwd -P newpassword &&
+	(
+		P4PASSWD=badpassword test_must_fail git p4 clone //depot/foo 2>errmsg &&
+		grep -q "failure accessing depot.*P4PASSWD" errmsg
+	)
+'
+
+test_expect_success 'ticket logged out' '
+	P4TICKETS="$cli/tickets" &&
+	echo "newpassword" | p4 login &&
+	(
+		cd "$git" &&
+		test_commit "ticket-auth-check" &&
+		p4 logout &&
+		test_must_fail git p4 submit 2>errmsg &&
+		grep -q "failure accessing depot" errmsg
+	)
+'
+
+test_expect_success 'create group with short ticket expiry' '
+	P4TICKETS="$cli/tickets" &&
+	echo "newpassword" | p4 login &&
+	p4_add_user short_expiry_user &&
+	p4 -u short_expiry_user passwd -P password &&
+	p4 group -i <<-EOF &&
+	Group: testgroup
+	Timeout: 3
+	Users: short_expiry_user
+	EOF
+
+	p4 users | grep short_expiry_user
+'
+
+test_expect_success 'git operation with expired ticket' '
+	P4TICKETS="$cli/tickets" &&
+	P4USER=short_expiry_user &&
+	echo "password" | p4 login &&
+	(
+		cd "$git" &&
+		git p4 sync &&
+		sleep 5 &&
+		test_must_fail git p4 sync 2>errmsg &&
+		grep "failure accessing depot" errmsg
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+
+test_done
-- 
2.17.0.392.gdeb1a6e9b7

Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails

[Luke Diamand]

On 5 June 2018 at 11:49, Merland Romain <merlorom@yahoo.fr> wrote:
>
>> @@ -91,6 +93,13 @@ def p4_build_cmd(cmd):
>>         real_cmd = ' '.join(real_cmd) + ' ' + cmd
>>     else:
>>         real_cmd += cmd
>> +
>> +    # now check that we can actually talk to the server
>> +    global p4_access_checked
>> +    if not p4_access_checked:
>> +        p4_access_checked = True
>> +        p4_check_access()
>> +
>
> Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'
> It seems to me more logical

Like this:

+        p4_check_access()
+        p4_access_checked = True

You need to set p4_access_checked first so that it doesn't go and try
to check the p4 access before running "p4 login -s", which would then
get stuck forever.

>
> Romain

Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails

[Eric Sunshine]

On Tue, Jun 5, 2018 at 6:59 AM Luke Diamand <luke@diamand.org> wrote:
> On 5 June 2018 at 11:49, Merland Romain <merlorom@yahoo.fr> wrote:
> >> +    # now check that we can actually talk to the server
> >> +    global p4_access_checked
> >> +    if not p4_access_checked:
> >> +        p4_access_checked = True
> >> +        p4_check_access()
> >> +
> >
> > Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'
> > It seems to me more logical
>
> Like this:
>
> +        p4_check_access()
> +        p4_access_checked = True
>
> You need to set p4_access_checked first so that it doesn't go and try
> to check the p4 access before running "p4 login -s", which would then
> get stuck forever.

Such subtlety may deserve an in-code comment.