{"thread":{"id":"45700","subject":"[PATCH 0/3] git-p4: use symbolic-ref instead of name-rev","startedAt":"2017-04-15T10:39:34Z","lastAt":"2017-04-15T10:39:41Z","messageCount":4,"participants":["Luke Diamand"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"316880","messageId":"20170415103609.6002-1-luke@diamand.org","threadId":"45700","inReplyTo":null,"subject":"[PATCH 0/3] git-p4: use symbolic-ref instead of name-rev","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-04-15T10:36:06Z","receivedAt":"2017-04-15T10:39:34Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Followup to earlier discussion about use of name-rev in git-p4.\n\nhttp://marc.info/?l=git&m=148979063421355\n\nLuke Diamand (3):\n  git-p4: add failing test for name-rev rather than symbolic-ref\n  git-p4: add read_pipe_text() internal function\n  git-p4: don't use name-rev to get current branch\n\n git-p4.py                | 38 +++++++++++++++++++++++++++++---------\n t/t9807-git-p4-submit.sh | 16 ++++++++++++++++\n 2 files changed, 45 insertions(+), 9 deletions(-)\n\n-- \n2.12.2.719.gcbd162c\n\n"},{"id":"316881","messageId":"20170415103609.6002-2-luke@diamand.org","threadId":"45700","inReplyTo":"20170415103609.6002-1-luke@diamand.org","subject":"[PATCH 1/3] git-p4: add failing test for name-rev rather than symbolic-ref","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-04-15T10:36:07Z","receivedAt":"2017-04-15T10:39:38Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Using name-rev to find the current git branch means that git-p4\ndoes not correctly get the current branch name if there are\nmultiple branches pointing at HEAD, or a tag.\n\nThis change adds a test case which demonstrates the problem.\nConfiguring which branches are allowed to be submitted from goes\nwrong, as git-p4 gets confused about which branch is in use.\n\nThis appears to be the only place that git-p4 actually cares\nabout the current branch.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t9807-git-p4-submit.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex e37239e65..ae05816e0 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -139,6 +139,22 @@ test_expect_success 'submit with master branch name from argv' '\n \t)\n '\n \n+test_expect_failure 'allow submit from branch with same revision but different name' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\ttest_commit \"file8\" &&\n+\t\tgit checkout -b branch1 &&\n+\t\tgit checkout -b branch2 &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit config git-p4.allowSubmit \"branch1\" &&\n+\t\ttest_must_fail git p4 submit &&\n+\t\tgit checkout branch1 &&\n+\t\tgit p4 submit\n+\t)\n+'\n+\n #\n # Basic submit tests, the five handled cases\n #\n-- \n2.12.2.719.gcbd162c\n\n"},{"id":"316882","messageId":"20170415103609.6002-3-luke@diamand.org","threadId":"45700","inReplyTo":"20170415103609.6002-1-luke@diamand.org","subject":"[PATCH 2/3] git-p4: add read_pipe_text() internal function","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-04-15T10:36:08Z","receivedAt":"2017-04-15T10:39:40Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"The existing read_pipe() function returns an empty string on\nerror, but also returns an empty string if the command returns\nan empty string.\n\nThis leads to ugly constructions trying to detect error cases.\n\nAdd read_pipe_text() which just returns None on error.\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n git-p4.py | 31 ++++++++++++++++++++++++++++---\n 1 file changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex eab319d76..584b81775 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -160,17 +160,42 @@ def p4_write_pipe(c, stdin):\n     real_cmd = p4_build_cmd(c)\n     return write_pipe(real_cmd, stdin)\n \n-def read_pipe(c, ignore_error=False):\n+def read_pipe_full(c):\n+    \"\"\" Read output from  command. Returns a tuple\n+        of the return status, stdout text and stderr\n+        text.\n+    \"\"\"\n     if verbose:\n         sys.stderr.write('Reading pipe: %s\\n' % str(c))\n \n     expand = isinstance(c,basestring)\n     p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)\n     (out, err) = p.communicate()\n-    if p.returncode != 0 and not ignore_error:\n-        die('Command failed: %s\\nError: %s' % (str(c), err))\n+    return (p.returncode, out, err)\n+\n+def read_pipe(c, ignore_error=False):\n+    \"\"\" Read output from  command. Returns the output text on\n+        success. On failure, terminates execution, unless\n+        ignore_error is True, when it returns an empty string.\n+    \"\"\"\n+    (retcode, out, err) = read_pipe_full(c)\n+    if retcode != 0:\n+        if ignore_error:\n+            out = \"\"\n+        else:\n+            die('Command failed: %s\\nError: %s' % (str(c), err))\n     return out\n \n+def read_pipe_text(c):\n+    \"\"\" Read output from a command with trailing whitespace stripped.\n+        On error, returns None.\n+    \"\"\"\n+    (retcode, out, err) = read_pipe_full(c)\n+    if retcode != 0:\n+        return None\n+    else:\n+        return out.rstrip()\n+\n def p4_read_pipe(c, ignore_error=False):\n     real_cmd = p4_build_cmd(c)\n     return read_pipe(real_cmd, ignore_error)\n-- \n2.12.2.719.gcbd162c\n\n"},{"id":"316883","messageId":"20170415103609.6002-4-luke@diamand.org","threadId":"45700","inReplyTo":"20170415103609.6002-1-luke@diamand.org","subject":"[PATCH 3/3] git-p4: don't use name-rev to get current branch","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2017-04-15T10:36:09Z","receivedAt":"2017-04-15T10:39:41Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"git-p4 was using \"git name-rev\" to find out the current branch.\n\nThat is not safe, since if multiple branches or tags point at\nthe same revision, the result obtained might not be what is\nexpected.\n\nInstead use \"git symbolic-ref\".\n\nSigned-off-by: Luke Diamand <luke@diamand.org>\n---\n git-p4.py                | 7 +------\n t/t9807-git-p4-submit.sh | 2 +-\n 2 files changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 584b81775..8d151da91 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -602,12 +602,7 @@ def p4Where(depotPath):\n     return clientPath\n \n def currentGitBranch():\n-    retcode = system([\"git\", \"symbolic-ref\", \"-q\", \"HEAD\"], ignore_error=True)\n-    if retcode != 0:\n-        # on a detached head\n-        return None\n-    else:\n-        return read_pipe([\"git\", \"name-rev\", \"HEAD\"]).split(\" \")[1].strip()\n+    return read_pipe_text([\"git\", \"symbolic-ref\", \"--short\", \"-q\", \"HEAD\"])\n \n def isValidGitDir(path):\n     return git_dir(path) != None\ndiff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh\nindex ae05816e0..3457d5db6 100755\n--- a/t/t9807-git-p4-submit.sh\n+++ b/t/t9807-git-p4-submit.sh\n@@ -139,7 +139,7 @@ test_expect_success 'submit with master branch name from argv' '\n \t)\n '\n \n-test_expect_failure 'allow submit from branch with same revision but different name' '\n+test_expect_success 'allow submit from branch with same revision but different name' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n \t(\n-- \n2.12.2.719.gcbd162c\n\n"}]}