* [PATCH 1/6] git p4: catch p4 describe errors
2012-11-23 22:35 [PATCH 0/6] git p4 error handling Pete Wyckoff
@ 2012-11-23 22:35 ` Pete Wyckoff
2012-11-23 22:35 ` [PATCH 2/6] git p4: handle servers without move support Pete Wyckoff
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Pete Wyckoff @ 2012-11-23 22:35 UTC (permalink / raw)
To: git
Group the two calls to "p4 describe" into a new helper function,
and try to validate the p4 results. The current behavior when p4
describe fails is to die with a python backtrace. The new behavior
will print the full response.
This does not solve any particular problem, but adds more
checking in hopes of narrowing down odd behavior seen on
at least two occasions.
Based-on-patch-by: Matt Arsenault <arsenm2@gmail.com>
Reported-by: Arthur <a.foulon@amesys.fr>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 7d6c928..cd68e04 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -169,6 +169,29 @@ def p4_reopen(type, f):
def p4_move(src, dest):
p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)])
+def p4_describe(change):
+ """Make sure it returns a valid result by checking for
+ the presence of field "time". Return a dict of the
+ results."""
+
+ ds = p4CmdList(["describe", "-s", str(change)])
+ if len(ds) != 1:
+ die("p4 describe -s %d did not return 1 result: %s" % (change, str(ds)))
+
+ d = ds[0]
+
+ if "p4ExitCode" in d:
+ die("p4 describe -s %d exited with %d: %s" % (change, d["p4ExitCode"],
+ str(d)))
+ if "code" in d:
+ if d["code"] == "error":
+ die("p4 describe -s %d returned error code: %s" % (change, str(d)))
+
+ if "time" not in d:
+ die("p4 describe -s %d returned no \"time\": %s" % (change, str(d)))
+
+ return d
+
#
# Canonicalize the p4 type and return a tuple of the
# base type, plus any modifiers. See "p4 help filetypes"
@@ -2543,7 +2566,7 @@ class P4Sync(Command, P4UserMap):
def importChanges(self, changes):
cnt = 1
for change in changes:
- description = p4Cmd(["describe", str(change)])
+ description = p4_describe(change)
self.updateOptionDict(description)
if not self.silent:
@@ -2667,14 +2690,8 @@ class P4Sync(Command, P4UserMap):
# Use time from top-most change so that all git p4 clones of
# the same p4 repo have the same commit SHA1s.
- res = p4CmdList("describe -s %d" % newestRevision)
- newestTime = None
- for r in res:
- if r.has_key('time'):
- newestTime = int(r['time'])
- if newestTime is None:
- die("\"describe -s\" on newest change %d did not give a time")
- details["time"] = newestTime
+ res = p4_describe(newestRevision)
+ details["time"] = res["time"]
self.updateOptionDict(details)
try:
--
1.8.0.276.gd9397fc
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/6] git p4: handle servers without move support
2012-11-23 22:35 [PATCH 0/6] git p4 error handling Pete Wyckoff
2012-11-23 22:35 ` [PATCH 1/6] git p4: catch p4 describe errors Pete Wyckoff
@ 2012-11-23 22:35 ` Pete Wyckoff
2012-11-23 22:35 ` [PATCH 3/6] git p4: catch p4 errors when streaming file contents Pete Wyckoff
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Pete Wyckoff @ 2012-11-23 22:35 UTC (permalink / raw)
To: git
Support for the "p4 move" command was added in 8e9497c (git p4:
add support for 'p4 move' in P4Submit, 2012-07-12), which checks
to make sure that the client and server support the command.
But older versions of p4d may not handle the "-k" argument, and
newer p4d allow disabling "p4 move" with a configuration setting.
Check for both these cases by testing a p4 move command on bogus
filenames and looking for strings in the error messages.
Reported-by: Vitor Antunes <vitor.hda@gmail.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 21 ++++++++++++++++++++-
t/t9814-git-p4-rename.sh | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index cd68e04..9644c9f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -129,6 +129,25 @@ def p4_has_command(cmd):
p.communicate()
return p.returncode == 0
+def p4_has_move_command():
+ """See if the move command exists, that it supports -k, and that
+ it has not been administratively disabled. The arguments
+ must be correct, but the filenames do not have to exist. Use
+ ones with wildcards so even if they exist, it will fail."""
+
+ if not p4_has_command("move"):
+ return False
+ cmd = p4_build_cmd(["move", "-k", "@from", "@to"])
+ p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+ (out, err) = p.communicate()
+ # return code will be 1 in either case
+ if err.find("Invalid option") >= 0:
+ return False
+ if err.find("disabled") >= 0:
+ return False
+ # assume it failed because @... was invalid changelist
+ return True
+
def system(cmd):
expand = isinstance(cmd,basestring)
if verbose:
@@ -894,7 +913,7 @@ class P4Submit(Command, P4UserMap):
self.conflict_behavior = None
self.isWindows = (platform.system() == "Windows")
self.exportLabels = False
- self.p4HasMoveCommand = p4_has_command("move")
+ self.p4HasMoveCommand = p4_has_move_command()
def check(self):
if len(p4CmdList("opened ...")) > 0:
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 3bf1224..be802e0 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -199,6 +199,41 @@ test_expect_success 'detect copies' '
)
'
+# See if configurables can be set, and in particular if the run.move.allow
+# variable exists, which allows admins to disable the "p4 move" command.
+test_expect_success 'p4 configure command and run.move.allow are available' '
+ p4 configure show run.move.allow >out ; retval=$? &&
+ test $retval = 0 &&
+ {
+ egrep ^run.move.allow: out &&
+ test_set_prereq P4D_HAVE_CONFIGURABLE_RUN_MOVE_ALLOW ||
+ true
+ } || true
+'
+
+# If move can be disabled, turn it off and test p4 move handling
+test_expect_success P4D_HAVE_CONFIGURABLE_RUN_MOVE_ALLOW \
+ 'do not use p4 move when administratively disabled' '
+ test_when_finished "p4 configure set run.move.allow=1" &&
+ p4 configure set run.move.allow=0 &&
+ (
+ cd "$cli" &&
+ echo move-disallow-file >move-disallow-file &&
+ p4 add move-disallow-file &&
+ p4 submit -d "add move-disallow-file"
+ ) &&
+ test_when_finished cleanup_git &&
+ git p4 clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.detectRenames true &&
+ git mv move-disallow-file move-disallow-file-moved &&
+ git commit -m "move move-disallow-file" &&
+ git p4 submit
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
1.8.0.276.gd9397fc
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/6] git p4: catch p4 errors when streaming file contents
2012-11-23 22:35 [PATCH 0/6] git p4 error handling Pete Wyckoff
2012-11-23 22:35 ` [PATCH 1/6] git p4: catch p4 describe errors Pete Wyckoff
2012-11-23 22:35 ` [PATCH 2/6] git p4: handle servers without move support Pete Wyckoff
@ 2012-11-23 22:35 ` Pete Wyckoff
2012-11-23 22:35 ` [PATCH 4/6] git p4 test: display unresolvable host error Pete Wyckoff
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Pete Wyckoff @ 2012-11-23 22:35 UTC (permalink / raw)
To: git
Error messages that arise during the "p4 print" phase of
generating commits were silently ignored. Catch them,
abort the fast-import, and exit.
Without this fix, the sync/clone appears to work, but files that
are inaccessible by the p4d server will still be imported to git,
although without the proper contents. Instead the errant files
will contain a p4 error message, such as "Librarian checkout
//depot/path failed".
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 38 +++++++++++++++++++++++++++++++-------
t/t9800-git-p4-basic.sh | 13 ++++++++++++-
2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 9644c9f..cb1ec8d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2139,6 +2139,29 @@ class P4Sync(Command, P4UserMap):
# handle another chunk of streaming data
def streamP4FilesCb(self, marshalled):
+ # catch p4 errors and complain
+ err = None
+ if "code" in marshalled:
+ if marshalled["code"] == "error":
+ if "data" in marshalled:
+ err = marshalled["data"].rstrip()
+ if err:
+ f = None
+ if self.stream_have_file_info:
+ if "depotFile" in self.stream_file:
+ f = self.stream_file["depotFile"]
+ # force a failure in fast-import, else an empty
+ # commit will be made
+ self.gitStream.write("\n")
+ self.gitStream.write("die-now\n")
+ self.gitStream.close()
+ # ignore errors, but make sure it exits first
+ self.importProcess.wait()
+ if f:
+ die("Error from p4 print for %s: %s" % (f, err))
+ else:
+ die("Error from p4 print: %s" % err)
+
if marshalled.has_key('depotFile') and self.stream_have_file_info:
# start of a new file - output the old one first
self.streamOneP4File(self.stream_file, self.stream_contents)
@@ -2900,12 +2923,13 @@ class P4Sync(Command, P4UserMap):
self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
- importProcess = subprocess.Popen(["git", "fast-import"],
- stdin=subprocess.PIPE, stdout=subprocess.PIPE,
- stderr=subprocess.PIPE);
- self.gitOutput = importProcess.stdout
- self.gitStream = importProcess.stdin
- self.gitError = importProcess.stderr
+ self.importProcess = subprocess.Popen(["git", "fast-import"],
+ stdin=subprocess.PIPE,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE);
+ self.gitOutput = self.importProcess.stdout
+ self.gitStream = self.importProcess.stdin
+ self.gitError = self.importProcess.stderr
if revision:
self.importHeadRevision(revision)
@@ -2965,7 +2989,7 @@ class P4Sync(Command, P4UserMap):
self.importP4Labels(self.gitStream, missingP4Labels)
self.gitStream.close()
- if importProcess.wait() != 0:
+ if self.importProcess.wait() != 0:
die("fast-import failed: %s" % self.gitError.read())
self.gitOutput.close()
self.gitError.close()
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index b7ad716..05797c3 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -143,7 +143,18 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
! test_i18ngrep Traceback errs
'
-test_expect_success 'clone bare' '
+# Hide a file from p4d, make sure we catch its complaint. This won't fail in
+# p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the
+# message will include "Librarian checkout".
+test_expect_success 'exit gracefully for p4 server errors' '
+ test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" &&
+ mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden &&
+ test_when_finished cleanup_git &&
+ test_expect_code 1 git p4 clone --dest="$git" //depot@1 >out 2>err &&
+ test_i18ngrep "Error from p4 print" err
+'
+
+test_expect_success 'clone --bare should make a bare repository' '
rm -rf "$git" &&
git p4 clone --dest="$git" --bare //depot &&
test_when_finished cleanup_git &&
--
1.8.0.276.gd9397fc
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/6] git p4 test: display unresolvable host error
2012-11-23 22:35 [PATCH 0/6] git p4 error handling Pete Wyckoff
` (2 preceding siblings ...)
2012-11-23 22:35 ` [PATCH 3/6] git p4: catch p4 errors when streaming file contents Pete Wyckoff
@ 2012-11-23 22:35 ` Pete Wyckoff
2012-11-23 22:35 ` [PATCH 5/6] git p4: fix labelDetails typo in exception Pete Wyckoff
2012-11-23 22:35 ` [PATCH 6/6] git p4: remove unneeded cmd initialization Pete Wyckoff
5 siblings, 0 replies; 7+ messages in thread
From: Pete Wyckoff @ 2012-11-23 22:35 UTC (permalink / raw)
To: git
This test passes already. Make sure p4 diagnostic errors are displayed.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9800-git-p4-basic.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 05797c3..8c59796 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -183,6 +183,18 @@ test_expect_success 'initial import time from top change time' '
)
'
+test_expect_success 'unresolvable host in P4PORT should display error' '
+ test_when_finished cleanup_git &&
+ git p4 clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ P4PORT=nosuchhost:65537 &&
+ export P4PORT &&
+ test_expect_code 1 git p4 sync >out 2>err &&
+ grep "connect to nosuchhost" err
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
1.8.0.276.gd9397fc
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 5/6] git p4: fix labelDetails typo in exception
2012-11-23 22:35 [PATCH 0/6] git p4 error handling Pete Wyckoff
` (3 preceding siblings ...)
2012-11-23 22:35 ` [PATCH 4/6] git p4 test: display unresolvable host error Pete Wyckoff
@ 2012-11-23 22:35 ` Pete Wyckoff
2012-11-23 22:35 ` [PATCH 6/6] git p4: remove unneeded cmd initialization Pete Wyckoff
5 siblings, 0 replies; 7+ messages in thread
From: Pete Wyckoff @ 2012-11-23 22:35 UTC (permalink / raw)
To: git
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index cb1ec8d..9712082 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2406,7 +2406,7 @@ class P4Sync(Command, P4UserMap):
try:
tmwhen = time.strptime(labelDetails['Update'], "%Y/%m/%d %H:%M:%S")
except ValueError:
- print "Could not convert label time %s" % labelDetail['Update']
+ print "Could not convert label time %s" % labelDetails['Update']
tmwhen = 1
when = int(time.mktime(tmwhen))
--
1.8.0.276.gd9397fc
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 6/6] git p4: remove unneeded cmd initialization
2012-11-23 22:35 [PATCH 0/6] git p4 error handling Pete Wyckoff
` (4 preceding siblings ...)
2012-11-23 22:35 ` [PATCH 5/6] git p4: fix labelDetails typo in exception Pete Wyckoff
@ 2012-11-23 22:35 ` Pete Wyckoff
5 siblings, 0 replies; 7+ messages in thread
From: Pete Wyckoff @ 2012-11-23 22:35 UTC (permalink / raw)
To: git
It confuses pylint, and is never needed.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 1 -
1 file changed, 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index 9712082..551aec9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3188,7 +3188,6 @@ def main():
printUsage(commands.keys())
sys.exit(2)
- cmd = ""
cmdName = sys.argv[1]
try:
klass = commands[cmdName]
--
1.8.0.276.gd9397fc
^ permalink raw reply related [flat|nested] 7+ messages in thread