# [PATCH 0/3] git-p4: use symbolic-ref instead of name-rev 4 messages from 2017-04-15 to 2017-04-15. Participants: Luke Diamand. Thread: https://gitlist.dev/t/45700 ## Luke Diamand, 2017-04-15 10:36 Subject: [PATCH 0/3] git-p4: use symbolic-ref instead of name-rev Message-ID: <20170415103609.6002-1-luke@diamand.org> URL: https://gitlist.dev/e/20170415103609.6002-1-luke%40diamand.org ``` Followup to earlier discussion about use of name-rev in git-p4. http://marc.info/?l=git&m=148979063421355 Luke Diamand (3): git-p4: add failing test for name-rev rather than symbolic-ref git-p4: add read_pipe_text() internal function git-p4: don't use name-rev to get current branch git-p4.py | 38 +++++++++++++++++++++++++++++--------- t/t9807-git-p4-submit.sh | 16 ++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) -- 2.12.2.719.gcbd162c ``` ## Luke Diamand, 2017-04-15 10:36 Subject: [PATCH 1/3] git-p4: add failing test for name-rev rather than symbolic-ref Message-ID: <20170415103609.6002-2-luke@diamand.org> URL: https://gitlist.dev/e/20170415103609.6002-2-luke%40diamand.org In-Reply-To: <20170415103609.6002-1-luke@diamand.org> ``` Using name-rev to find the current git branch means that git-p4 does not correctly get the current branch name if there are multiple branches pointing at HEAD, or a tag. This change adds a test case which demonstrates the problem. Configuring which branches are allowed to be submitted from goes wrong, as git-p4 gets confused about which branch is in use. This appears to be the only place that git-p4 actually cares about the current branch. Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- t/t9807-git-p4-submit.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index e37239e65..ae05816e0 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -139,6 +139,22 @@ test_expect_success 'submit with master branch name from argv' ' ) ' +test_expect_failure 'allow submit from branch with same revision but different name' ' + test_when_finished cleanup_git && + git p4 clone --dest="$git" //depot && + ( + cd "$git" && + test_commit "file8" && + git checkout -b branch1 && + git checkout -b branch2 && + git config git-p4.skipSubmitEdit true && + git config git-p4.allowSubmit "branch1" && + test_must_fail git p4 submit && + git checkout branch1 && + git p4 submit + ) +' + # # Basic submit tests, the five handled cases # -- 2.12.2.719.gcbd162c ``` ## Luke Diamand, 2017-04-15 10:36 Subject: [PATCH 2/3] git-p4: add read_pipe_text() internal function Message-ID: <20170415103609.6002-3-luke@diamand.org> URL: https://gitlist.dev/e/20170415103609.6002-3-luke%40diamand.org In-Reply-To: <20170415103609.6002-1-luke@diamand.org> ``` The existing read_pipe() function returns an empty string on error, but also returns an empty string if the command returns an empty string. This leads to ugly constructions trying to detect error cases. Add read_pipe_text() which just returns None on error. Signed-off-by: Luke Diamand --- git-p4.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/git-p4.py b/git-p4.py index eab319d76..584b81775 100755 --- a/git-p4.py +++ b/git-p4.py @@ -160,17 +160,42 @@ def p4_write_pipe(c, stdin): real_cmd = p4_build_cmd(c) return write_pipe(real_cmd, stdin) -def read_pipe(c, ignore_error=False): +def read_pipe_full(c): + """ Read output from command. Returns a tuple + of the return status, stdout text and stderr + text. + """ if verbose: sys.stderr.write('Reading pipe: %s\n' % str(c)) expand = isinstance(c,basestring) p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand) (out, err) = p.communicate() - if p.returncode != 0 and not ignore_error: - die('Command failed: %s\nError: %s' % (str(c), err)) + return (p.returncode, out, err) + +def read_pipe(c, ignore_error=False): + """ Read output from command. Returns the output text on + success. On failure, terminates execution, unless + ignore_error is True, when it returns an empty string. + """ + (retcode, out, err) = read_pipe_full(c) + if retcode != 0: + if ignore_error: + out = "" + else: + die('Command failed: %s\nError: %s' % (str(c), err)) return out +def read_pipe_text(c): + """ Read output from a command with trailing whitespace stripped. + On error, returns None. + """ + (retcode, out, err) = read_pipe_full(c) + if retcode != 0: + return None + else: + return out.rstrip() + def p4_read_pipe(c, ignore_error=False): real_cmd = p4_build_cmd(c) return read_pipe(real_cmd, ignore_error) -- 2.12.2.719.gcbd162c ``` ## Luke Diamand, 2017-04-15 10:36 Subject: [PATCH 3/3] git-p4: don't use name-rev to get current branch Message-ID: <20170415103609.6002-4-luke@diamand.org> URL: https://gitlist.dev/e/20170415103609.6002-4-luke%40diamand.org In-Reply-To: <20170415103609.6002-1-luke@diamand.org> ``` git-p4 was using "git name-rev" to find out the current branch. That is not safe, since if multiple branches or tags point at the same revision, the result obtained might not be what is expected. Instead use "git symbolic-ref". Signed-off-by: Luke Diamand --- git-p4.py | 7 +------ t/t9807-git-p4-submit.sh | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/git-p4.py b/git-p4.py index 584b81775..8d151da91 100755 --- a/git-p4.py +++ b/git-p4.py @@ -602,12 +602,7 @@ def p4Where(depotPath): return clientPath def currentGitBranch(): - retcode = system(["git", "symbolic-ref", "-q", "HEAD"], ignore_error=True) - if retcode != 0: - # on a detached head - return None - else: - return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip() + return read_pipe_text(["git", "symbolic-ref", "--short", "-q", "HEAD"]) def isValidGitDir(path): return git_dir(path) != None diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index ae05816e0..3457d5db6 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -139,7 +139,7 @@ test_expect_success 'submit with master branch name from argv' ' ) ' -test_expect_failure 'allow submit from branch with same revision but different name' ' +test_expect_success 'allow submit from branch with same revision but different name' ' test_when_finished cleanup_git && git p4 clone --dest="$git" //depot && ( -- 2.12.2.719.gcbd162c ```