From 837b3a6376804ec70b88f06c3f702a38c59196c3 Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 29 Jan 2020 11:12:41 +0000 Subject: [PATCH 1/7] git-p4: make closeStreams() idempotent Ensure that we can safely call self.closeStreams() multiple times, and can also call it even if there is no git fast-import stream at all. Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- git-p4.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git-p4.py b/git-p4.py index 40d9e7c594..23724defe8 100755 --- a/git-p4.py +++ b/git-p4.py @@ -3549,11 +3549,14 @@ class P4Sync(Command, P4UserMap): self.gitError = self.importProcess.stderr def closeStreams(self): + if self.gitStream is None: + return self.gitStream.close() if self.importProcess.wait() != 0: die("fast-import failed: %s" % self.gitError.read()) self.gitOutput.close() self.gitError.close() + self.gitStream = None def run(self, args): if self.importIntoRemotes: From 5c3d5020e66caad0418b5696ba0f3d013641fcdd Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 29 Jan 2020 11:12:42 +0000 Subject: [PATCH 2/7] git-p4: add P4CommandException to report errors talking to Perforce Currently when there is a P4 error, git-p4 calls die() which just exits. This then leaves the git-fast-import process still running, and can even leave p4 itself still running. As a result, git-p4 fails to exit cleanly. This is a particular problem for people running the unit tests in regression. Use this exception to report errors upwards, cleaning up as the error propagates. Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- git-p4.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/git-p4.py b/git-p4.py index 23724defe8..df2a956622 100755 --- a/git-p4.py +++ b/git-p4.py @@ -618,6 +618,14 @@ class P4RequestSizeException(P4ServerException): super(P4RequestSizeException, self).__init__(exit_code, p4_result) self.limit = limit +class P4CommandException(P4Exception): + """ Something went wrong calling p4 which means we have to give up """ + def __init__(self, msg): + self.msg = msg + + def __str__(self): + return self.msg + def isModeExecChanged(src_mode, dst_mode): return isModeExec(src_mode) != isModeExec(dst_mode) From 4c1d58675d07296adb2cf40b4b1c24f8d7a20d75 Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 29 Jan 2020 11:12:43 +0000 Subject: [PATCH 3/7] git-p4: disable some pylint warnings, to get pylint output to something manageable pylint is incredibly useful for finding bugs, but git-p4 has never used it, so there are a lot of warnings that while important, don't actually result in bugs. Let's turn those off for now, so we can get some useful output. Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- git-p4.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/git-p4.py b/git-p4.py index df2a956622..d796074b87 100755 --- a/git-p4.py +++ b/git-p4.py @@ -7,6 +7,14 @@ # 2007 Trolltech ASA # License: MIT # +# pylint: disable=invalid-name,missing-docstring,too-many-arguments,broad-except +# pylint: disable=no-self-use,wrong-import-position,consider-iterating-dictionary +# pylint: disable=wrong-import-order,unused-import,too-few-public-methods +# pylint: disable=too-many-lines,ungrouped-imports,fixme,too-many-locals +# pylint: disable=line-too-long,bad-whitespace,superfluous-parens +# pylint: disable=too-many-statements,too-many-instance-attributes +# pylint: disable=too-many-branches,too-many-nested-blocks +# import sys if sys.hexversion < 0x02040000: # The limiter is the subprocess module From ca5b5cce6290351f8cb63ee4ff466ed4a2285319 Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 29 Jan 2020 11:12:44 +0000 Subject: [PATCH 4/7] git-p4: create helper function importRevisions() This makes it easier to try/catch around this block of code to ensure cleanup following p4 failures is handled properly. Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- git-p4.py | 132 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 68 insertions(+), 64 deletions(-) diff --git a/git-p4.py b/git-p4.py index d796074b87..f90b43fe5e 100755 --- a/git-p4.py +++ b/git-p4.py @@ -3555,6 +3555,73 @@ class P4Sync(Command, P4UserMap): print("IO error details: {}".format(err)) print(self.gitError.read()) + + def importRevisions(self, args, branch_arg_given): + changes = [] + + if len(self.changesFile) > 0: + output = open(self.changesFile).readlines() + changeSet = set() + for line in output: + changeSet.add(int(line)) + + for change in changeSet: + changes.append(change) + + changes.sort() + else: + # catch "git p4 sync" with no new branches, in a repo that + # does not have any existing p4 branches + if len(args) == 0: + if not self.p4BranchesInGit: + die("No remote p4 branches. Perhaps you never did \"git p4 clone\" in here.") + + # The default branch is master, unless --branch is used to + # specify something else. Make sure it exists, or complain + # nicely about how to use --branch. + if not self.detectBranches: + if not branch_exists(self.branch): + if branch_arg_given: + die("Error: branch %s does not exist." % self.branch) + else: + die("Error: no branch %s; perhaps specify one with --branch." % + self.branch) + + if self.verbose: + print("Getting p4 changes for %s...%s" % (', '.join(self.depotPaths), + self.changeRange)) + changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size) + + if len(self.maxChanges) > 0: + changes = changes[:min(int(self.maxChanges), len(changes))] + + if len(changes) == 0: + if not self.silent: + print("No changes to import!") + else: + if not self.silent and not self.detectBranches: + print("Import destination: %s" % self.branch) + + self.updatedBranches = set() + + if not self.detectBranches: + if args: + # start a new branch + self.initialParent = "" + else: + # build on a previous revision + self.initialParent = parseRevision(self.branch) + + self.importChanges(changes) + + if not self.silent: + print("") + if len(self.updatedBranches) > 0: + sys.stdout.write("Updated branches: ") + for b in self.updatedBranches: + sys.stdout.write("%s " % b) + sys.stdout.write("\n") + def openStreams(self): self.importProcess = subprocess.Popen(["git", "fast-import"], stdin=subprocess.PIPE, @@ -3761,70 +3828,7 @@ class P4Sync(Command, P4UserMap): if revision: self.importHeadRevision(revision) else: - changes = [] - - if len(self.changesFile) > 0: - output = open(self.changesFile).readlines() - changeSet = set() - for line in output: - changeSet.add(int(line)) - - for change in changeSet: - changes.append(change) - - changes.sort() - else: - # catch "git p4 sync" with no new branches, in a repo that - # does not have any existing p4 branches - if len(args) == 0: - if not self.p4BranchesInGit: - die("No remote p4 branches. Perhaps you never did \"git p4 clone\" in here.") - - # The default branch is master, unless --branch is used to - # specify something else. Make sure it exists, or complain - # nicely about how to use --branch. - if not self.detectBranches: - if not branch_exists(self.branch): - if branch_arg_given: - die("Error: branch %s does not exist." % self.branch) - else: - die("Error: no branch %s; perhaps specify one with --branch." % - self.branch) - - if self.verbose: - print("Getting p4 changes for %s...%s" % (', '.join(self.depotPaths), - self.changeRange)) - changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size) - - if len(self.maxChanges) > 0: - changes = changes[:min(int(self.maxChanges), len(changes))] - - if len(changes) == 0: - if not self.silent: - print("No changes to import!") - else: - if not self.silent and not self.detectBranches: - print("Import destination: %s" % self.branch) - - self.updatedBranches = set() - - if not self.detectBranches: - if args: - # start a new branch - self.initialParent = "" - else: - # build on a previous revision - self.initialParent = parseRevision(self.branch) - - self.importChanges(changes) - - if not self.silent: - print("") - if len(self.updatedBranches) > 0: - sys.stdout.write("Updated branches: ") - for b in self.updatedBranches: - sys.stdout.write("%s " % b) - sys.stdout.write("\n") + self.importRevisions(args, branch_arg_given) if gitConfigBool("git-p4.importLabels"): self.importLabels = True From 6026aff5bbe7389fa276188237b58afbef1b07ff Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 29 Jan 2020 11:12:45 +0000 Subject: [PATCH 5/7] git-p4: cleanup better on error exit After an error, git-p4 calls die(). This just exits, and leaves child processes still running. Instead of calling die(), raise an exception and catch it where the child process(es) (git-fastimport) are created. This was analyzed in detail here: https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/ This change does not address the particular issue of p4CmdList() invoking a subchild and not waiting for it on error. Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- git-p4.py | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/git-p4.py b/git-p4.py index f90b43fe5e..a69a24bf4c 100755 --- a/git-p4.py +++ b/git-p4.py @@ -169,6 +169,9 @@ def calcDiskFree(): return st.f_bavail * st.f_frsize def die(msg): + """ Terminate execution. Make sure that any running child processes have been wait()ed for before + calling this. + """ if verbose: raise Exception(msg) else: @@ -3574,7 +3577,7 @@ class P4Sync(Command, P4UserMap): # does not have any existing p4 branches if len(args) == 0: if not self.p4BranchesInGit: - die("No remote p4 branches. Perhaps you never did \"git p4 clone\" in here.") + raise P4CommandException("No remote p4 branches. Perhaps you never did \"git p4 clone\" in here.") # The default branch is master, unless --branch is used to # specify something else. Make sure it exists, or complain @@ -3582,9 +3585,9 @@ class P4Sync(Command, P4UserMap): if not self.detectBranches: if not branch_exists(self.branch): if branch_arg_given: - die("Error: branch %s does not exist." % self.branch) + raise P4CommandException("Error: branch %s does not exist." % self.branch) else: - die("Error: no branch %s; perhaps specify one with --branch." % + raise P4CommandException("Error: no branch %s; perhaps specify one with --branch." % self.branch) if self.verbose: @@ -3825,22 +3828,32 @@ class P4Sync(Command, P4UserMap): self.openStreams() - if revision: - self.importHeadRevision(revision) - else: - self.importRevisions(args, branch_arg_given) + err = None - if gitConfigBool("git-p4.importLabels"): - self.importLabels = True + try: + if revision: + self.importHeadRevision(revision) + else: + self.importRevisions(args, branch_arg_given) - if self.importLabels: - p4Labels = getP4Labels(self.depotPaths) - gitTags = getGitTags() + if gitConfigBool("git-p4.importLabels"): + self.importLabels = True - missingP4Labels = p4Labels - gitTags - self.importP4Labels(self.gitStream, missingP4Labels) + if self.importLabels: + p4Labels = getP4Labels(self.depotPaths) + gitTags = getGitTags() - self.closeStreams() + missingP4Labels = p4Labels - gitTags + self.importP4Labels(self.gitStream, missingP4Labels) + + except P4CommandException as e: + err = e + + finally: + self.closeStreams() + + if err: + die(str(err)) # Cleanup temporary branches created during import if self.tempBranches != []: From 19fa5ac333134fc3aa1e462780c4690177474ade Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Wed, 29 Jan 2020 11:12:46 +0000 Subject: [PATCH 6/7] git-p4: check for access to remote host earlier Check we can talk to the remote host before starting the git-fastimport subchild. Otherwise we fail to connect, and then exit, leaving git-fastimport still running since we did not wait() for it. Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- git-p4.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/git-p4.py b/git-p4.py index a69a24bf4c..eb5bc28cf9 100755 --- a/git-p4.py +++ b/git-p4.py @@ -3826,6 +3826,8 @@ class P4Sync(Command, P4UserMap): b = b[len(self.projectName):] self.createdBranches.add(b) + p4_check_access() + self.openStreams() err = None From 43f33e492af65d008045ee0695d0f17434e8c3c9 Mon Sep 17 00:00:00 2001 From: Luke Diamand Date: Thu, 30 Jan 2020 11:50:34 +0000 Subject: [PATCH 7/7] git-p4: avoid leak of file handle when cloning Spotted by Eric Sunshine: https://public-inbox.org/git/CAPig+cRx3hG64nuDie69o_gdX39F=sR6D8LyA7J1rCErgu0aMA@mail.gmail.com/ Signed-off-by: Luke Diamand Signed-off-by: Junio C Hamano --- git-p4.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-p4.py b/git-p4.py index eb5bc28cf9..9a71a6690d 100755 --- a/git-p4.py +++ b/git-p4.py @@ -3563,7 +3563,8 @@ class P4Sync(Command, P4UserMap): changes = [] if len(self.changesFile) > 0: - output = open(self.changesFile).readlines() + with open(self.changesFile) as f: + output = f.readlines() changeSet = set() for line in output: changeSet.add(int(line))