{"thread":{"id":"48653","subject":"[PATCHv1 0/1] git-p4: better error reporting","startedAt":"2018-06-05T08:55:37Z","lastAt":"2018-06-05T19:44:55Z","messageCount":4,"participants":["Luke Diamand","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"349323","messageId":"20180605085524.10838-1-luke@diamand.org","threadId":"48653","inReplyTo":null,"subject":"[PATCHv1 0/1] git-p4: better error reporting","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-06-05T08:55:23Z","receivedAt":"2018-06-05T08:55:37Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"If git-p4 cannot talk to the Perforce server, it will either give a\nconfusing error message, or just crash. Even I get tripped up by this.\n\nThis change just checks that the client can talk to the server, and in\nparticular that the user is logged in (the default login timeout is 12\nhours).\n\nIt would be nice to also check for ticket expiration during long\nPerforce operations, but this change does not attempt to do that.\n\nLuke Diamand (1):\n  git-p4: better error reporting when p4 fails\n\n git-p4.py         | 55 +++++++++++++++++++++++++++++++++\n t/t9833-errors.sh | 78 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 133 insertions(+)\n create mode 100755 t/t9833-errors.sh\n\n-- \n2.17.0.392.gdeb1a6e9b7\n\n"},{"id":"349324","messageId":"20180605085524.10838-2-luke@diamand.org","threadId":"48653","inReplyTo":"20180605085524.10838-1-luke@diamand.org","subject":"[PATCHv1 1/1] git-p4: better error reporting when p4 fails","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-06-05T08:55:24Z","receivedAt":"2018-06-05T08:55:40Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Currently when p4 fails to run, git-p4 just crashes with an obscure\nerror message.\n\nFor example, if the P4 ticket has expired, you get:\n\n  Error: Cannot locate perforce checkout of <path> in client view\n\nThis change checks whether git-p4 can talk to the Perforce server when\nthe first P4 operation is attempted, and tries to print a meaningful\nerror message if it fails.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n git-p4.py         | 55 +++++++++++++++++++++++++++++++++\n t/t9833-errors.sh | 78 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 133 insertions(+)\n create mode 100755 t/t9833-errors.sh\n\ndiff --git a/git-p4.py b/git-p4.py\nindex b9e79f1d8b..66652f8280 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -50,6 +50,8 @@ def __str__(self):\n # Grab changes in blocks of this many revisions, unless otherwise requested\n defaultBlockSize = 512\n \n+p4_access_checked = False\n+\n def p4_build_cmd(cmd):\n     \"\"\"Build a suitable p4 command line.\n \n@@ -91,6 +93,13 @@ def p4_build_cmd(cmd):\n         real_cmd = ' '.join(real_cmd) + ' ' + cmd\n     else:\n         real_cmd += cmd\n+\n+    # now check that we can actually talk to the server\n+    global p4_access_checked\n+    if not p4_access_checked:\n+        p4_access_checked = True\n+        p4_check_access()\n+\n     return real_cmd\n \n def git_dir(path):\n@@ -264,6 +273,52 @@ def p4_system(cmd):\n     if retcode:\n         raise CalledProcessError(retcode, real_cmd)\n \n+def die_bad_access(s):\n+    die(\"failure accessing depot: {0}\".format(s.rstrip()))\n+\n+def p4_check_access(min_expiration=1):\n+    \"\"\" Check if we can access Perforce - account still logged in\n+    \"\"\"\n+    results = p4CmdList([\"login\", \"-s\"])\n+\n+    if len(results) == 0:\n+        # should never get here: always get either some results, or a p4ExitCode\n+        assert(\"could not parse response from perforce\")\n+\n+    result = results[0]\n+\n+    if 'p4ExitCode' in result:\n+        # p4 returned non-zero status, e.g. P4PORT invalid, or p4 not in path\n+        die_bad_access(\"could not run p4\")\n+\n+    code = result.get(\"code\")\n+    if not code:\n+        # we get here if we couldn't connect and there was nothing to unmarshal\n+        die_bad_access(\"could not connect\")\n+\n+    elif code == \"stat\":\n+        expiry = result.get(\"TicketExpiration\")\n+        if expiry:\n+            expiry = int(expiry)\n+            if expiry > min_expiration:\n+                # ok to carry on\n+                return\n+            else:\n+                die_bad_access(\"perforce ticket expires in {0} seconds\".format(expiry))\n+\n+        else:\n+            # account without a timeout - all ok\n+            return\n+\n+    elif code == \"error\":\n+        data = result.get(\"data\")\n+        if data:\n+            die_bad_access(\"p4 error: {0}\".format(data))\n+        else:\n+            die_bad_access(\"unknown error\")\n+    else:\n+        die_bad_access(\"unknown error code {0}\".format(code))\n+\n _p4_version_string = None\n def p4_version_string():\n     \"\"\"Read the version string, showing just the last line, which\ndiff --git a/t/t9833-errors.sh b/t/t9833-errors.sh\nnew file mode 100755\nindex 0000000000..9ba892de7a\n--- /dev/null\n+++ b/t/t9833-errors.sh\n@@ -0,0 +1,78 @@\n+#!/bin/sh\n+\n+test_description='git p4 errors'\n+\n+. ./lib-git-p4.sh\n+\n+test_expect_success 'start p4d' '\n+\tstart_p4d\n+'\n+\n+test_expect_success 'add p4 files' '\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\techo file1 >file1 &&\n+\t\tp4 add file1 &&\n+\t\tp4 submit -d \"file1\"\n+\t)\n+'\n+\n+# after this test, the default user requires a password\n+test_expect_success 'error handling' '\n+\tgit p4 clone --dest=\"$git\" //depot@all &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tP4PORT=: test_must_fail git p4 submit 2>errmsg\n+\t) &&\n+\tp4 passwd -P newpassword &&\n+\t(\n+\t\tP4PASSWD=badpassword test_must_fail git p4 clone //depot/foo 2>errmsg &&\n+\t\tgrep -q \"failure accessing depot.*P4PASSWD\" errmsg\n+\t)\n+'\n+\n+test_expect_success 'ticket logged out' '\n+\tP4TICKETS=\"$cli/tickets\" &&\n+\techo \"newpassword\" | p4 login &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\ttest_commit \"ticket-auth-check\" &&\n+\t\tp4 logout &&\n+\t\ttest_must_fail git p4 submit 2>errmsg &&\n+\t\tgrep -q \"failure accessing depot\" errmsg\n+\t)\n+'\n+\n+test_expect_success 'create group with short ticket expiry' '\n+\tP4TICKETS=\"$cli/tickets\" &&\n+\techo \"newpassword\" | p4 login &&\n+\tp4_add_user short_expiry_user &&\n+\tp4 -u short_expiry_user passwd -P password &&\n+\tp4 group -i <<-EOF &&\n+\tGroup: testgroup\n+\tTimeout: 3\n+\tUsers: short_expiry_user\n+\tEOF\n+\n+\tp4 users | grep short_expiry_user\n+'\n+\n+test_expect_success 'git operation with expired ticket' '\n+\tP4TICKETS=\"$cli/tickets\" &&\n+\tP4USER=short_expiry_user &&\n+\techo \"password\" | p4 login &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit p4 sync &&\n+\t\tsleep 5 &&\n+\t\ttest_must_fail git p4 sync 2>errmsg &&\n+\t\tgrep \"failure accessing depot\" errmsg\n+\t)\n+'\n+\n+test_expect_success 'kill p4d' '\n+\tkill_p4d\n+'\n+\n+\n+test_done\n-- \n2.17.0.392.gdeb1a6e9b7\n\n"},{"id":"349332","messageId":"CAE5ih7_01ZixwnFP2kGN9Y5M26MyNai82USUr2UfunL3Ubztdg@mail.gmail.com","threadId":"48653","inReplyTo":"777690205.383623.1528195798488@mail.yahoo.com","subject":"Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-06-05T10:59:41Z","receivedAt":"2018-06-05T10:59:45Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 5 June 2018 at 11:49, Merland Romain <merlorom@yahoo.fr> wrote:\n>\n>> @@ -91,6 +93,13 @@ def p4_build_cmd(cmd):\n>>         real_cmd = ' '.join(real_cmd) + ' ' + cmd\n>>     else:\n>>         real_cmd += cmd\n>> +\n>> +    # now check that we can actually talk to the server\n>> +    global p4_access_checked\n>> +    if not p4_access_checked:\n>> +        p4_access_checked = True\n>> +        p4_check_access()\n>> +\n>\n> Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'\n> It seems to me more logical\n\nLike this:\n\n+        p4_check_access()\n+        p4_access_checked = True\n\nYou need to set p4_access_checked first so that it doesn't go and try\nto check the p4 access before running \"p4 login -s\", which would then\nget stuck forever.\n\n>\n> Romain\n"},{"id":"349381","messageId":"CAPig+cSP5RJedaoO1T=j4qmbAGmrNh_bJkamKcJRS+kF5wq0JQ@mail.gmail.com","threadId":"48653","inReplyTo":"CAE5ih7_01ZixwnFP2kGN9Y5M26MyNai82USUr2UfunL3Ubztdg@mail.gmail.com","subject":"Re: [PATCHv1 1/1] git-p4: better error reporting when p4 fails","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-05T19:44:40Z","receivedAt":"2018-06-05T19:44:55Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 5, 2018 at 6:59 AM Luke Diamand <luke@diamand.org> wrote:\n> On 5 June 2018 at 11:49, Merland Romain <merlorom@yahoo.fr> wrote:\n> >> +    # now check that we can actually talk to the server\n> >> +    global p4_access_checked\n> >> +    if not p4_access_checked:\n> >> +        p4_access_checked = True\n> >> +        p4_check_access()\n> >> +\n> >\n> > Just switch the 2 lines 'p4_access_checked = True' and 'p4_check_access()'\n> > It seems to me more logical\n>\n> Like this:\n>\n> +        p4_check_access()\n> +        p4_access_checked = True\n>\n> You need to set p4_access_checked first so that it doesn't go and try\n> to check the p4 access before running \"p4 login -s\", which would then\n> get stuck forever.\n\nSuch subtlety may deserve an in-code comment.\n"}]}