[PATCH 0/6] git p4 error handling

[PATCH 0/6] git p4 error handling

[Pete Wyckoff]

These are assorted minor fixes.  They should be safe for 1.8.1,
and are not urgent enough for maint.

Four of them deal with handling errors from p4d better.  The first two
of these have been seen on the list already.  The third showed up in my
own testing.  The fourth adds a test to make sure that existing error
handling continues to work.

  git p4: catch p4 describe errors
  git p4: handle servers without move support
  git p4: catch p4 errors when streaming file contents
  git p4 test: display unresolvable host error

These two are small cleanups found by pylint, one of which is an
obvious (but rare) bug.

  git p4: fix labelDetails typo in exception
  git p4: remove unneeded cmd initialization

 git-p4.py                | 97 ++++++++++++++++++++++++++++++++++++++----------
 t/t9800-git-p4-basic.sh  | 25 ++++++++++++-
 t/t9814-git-p4-rename.sh | 35 +++++++++++++++++
 3 files changed, 137 insertions(+), 20 deletions(-)

-- 
1.8.0.276.gd9397fc

[PATCH 1/6] git p4: catch p4 describe errors

[Pete Wyckoff]

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

[PATCH 2/6] git p4: handle servers without move support

[Pete Wyckoff]

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

[PATCH 3/6] git p4: catch p4 errors when streaming file contents

[Pete Wyckoff]

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

[PATCH 4/6] git p4 test: display unresolvable host error

[Pete Wyckoff]

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

[PATCH 5/6] git p4: fix labelDetails typo in exception

[Pete Wyckoff]

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

[PATCH 6/6] git p4: remove unneeded cmd initialization

[Pete Wyckoff]

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