From 18fa13d0b34b6243d3679ea78325ee33ee4d0989 Mon Sep 17 00:00:00 2001 From: Pete Wyckoff Date: Fri, 23 Nov 2012 17:35:34 -0500 Subject: git p4: catch p4 describe errors 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 Reported-by: Arthur Signed-off-by: Pete Wyckoff Signed-off-by: Junio C Hamano --- git-p4.py | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/git-p4.py b/git-p4.py index 7d6c928c3f..cd68e04a45 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: -- cgit v1.2.3 From 249da4c0dcd0534f416e2d5da0a9923c6068e492 Mon Sep 17 00:00:00 2001 From: Pete Wyckoff Date: Fri, 23 Nov 2012 17:35:35 -0500 Subject: git p4: handle servers without move support 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 Signed-off-by: Pete Wyckoff Signed-off-by: Junio C Hamano --- 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 cd68e04a45..9644c9f879 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 3bf1224ae0..be802e0e16 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 ' -- cgit v1.2.3 From 78189bead3f5fde22ae651d66208a0e0a375a819 Mon Sep 17 00:00:00 2001 From: Pete Wyckoff Date: Fri, 23 Nov 2012 17:35:36 -0500 Subject: git p4: catch p4 errors when streaming file contents 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 Signed-off-by: Junio C Hamano --- 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 9644c9f879..cb1ec8d66d 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 b7ad716b09..05797c31d1 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 && -- cgit v1.2.3 From e6777fde8d0ede84202ed1cc5ec0a175e0e52ab0 Mon Sep 17 00:00:00 2001 From: Pete Wyckoff Date: Fri, 23 Nov 2012 17:35:37 -0500 Subject: git p4 test: display unresolvable host error This test passes already. Make sure p4 diagnostic errors are displayed. Signed-off-by: Pete Wyckoff Signed-off-by: Junio C Hamano --- 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 05797c31d1..8c5979647f 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 ' -- cgit v1.2.3 From a4e9054cfbd869f96720e5ba4ce4d3a50e17141e Mon Sep 17 00:00:00 2001 From: Pete Wyckoff Date: Fri, 23 Nov 2012 17:35:38 -0500 Subject: git p4: fix labelDetails typo in exception Signed-off-by: Pete Wyckoff Signed-off-by: Junio C Hamano --- git-p4.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-p4.py b/git-p4.py index cb1ec8d66d..9712082a4f 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)) -- cgit v1.2.3 From 73350fb6aa7d0caf6c0e3ab06b73771b282de39d Mon Sep 17 00:00:00 2001 From: Pete Wyckoff Date: Fri, 23 Nov 2012 17:35:39 -0500 Subject: git p4: remove unneeded cmd initialization It confuses pylint, and is never needed. Signed-off-by: Pete Wyckoff Signed-off-by: Junio C Hamano --- git-p4.py | 1 - 1 file changed, 1 deletion(-) diff --git a/git-p4.py b/git-p4.py index 9712082a4f..551aec9417 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] -- cgit v1.2.3