{"thread":{"id":"52711","subject":"[PATCHv1 0/6] git-p4: wait() for child processes better","startedAt":"2020-01-29T11:12:54Z","lastAt":"2020-01-31T10:37:03Z","messageCount":13,"participants":["Luke Diamand","Eric Sunshine","Johannes Schindelin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"390713","messageId":"20200129111246.12196-1-luke@diamand.org","threadId":"52711","inReplyTo":null,"subject":"[PATCHv1 0/6] git-p4: wait() for child processes better","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T11:12:40Z","receivedAt":"2020-01-29T11:12:54Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"git-p4 handles most errors by calling die(). This can leave child\nprocesses still running, orphaned.\n\n https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/\n\nThis is not a problem for humans, but for CI, it is.\n\nThis change improves things by raising an exception and cleaning up\nfurther up the stack, rather than simply calling die().\n\nThis is only done in a few places, such that the tests pass with the changes\nsuggested in the link (adding sleep strategically) but there are still\nplenty of places where git-p4 calls die().\n\nThis also adds some pylint disables, so that we can start to run pylint\non git-p4.\n\nLuke Diamand (6):\n git-p4: make closeStreams() idempotent\n git-p4: add P4CommandException to report errors talking to Perforce\n git-p4: disable some pylint warnings, to get pylint output to\n something manageable\n git-p4: create helper function importRevisions()\n git-p4: cleanup better on error exit\n git-p4: check for access to remote host earlier\n\n git-p4.py | 180 +++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 109 insertions(+), 71 deletions(-)\n\n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"390714","messageId":"20200129111246.12196-2-luke@diamand.org","threadId":"52711","inReplyTo":"20200129111246.12196-1-luke@diamand.org","subject":"[PATCHv1 1/6] git-p4: make closeStreams() idempotent","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T11:12:41Z","receivedAt":"2020-01-29T11:12:55Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Ensure that we can safely call self.closeStreams() multiple times, and\ncan also call it even if there is no git fast-import stream at all.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 40d9e7c594..23724defe8 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3549,11 +3549,14 @@ def openStreams(self):\n self.gitError = self.importProcess.stderr\n \n def closeStreams(self):\n+ if self.gitStream is None:\n+ return\n self.gitStream.close()\n if self.importProcess.wait() != 0:\n die(\"fast-import failed: %s\" % self.gitError.read())\n self.gitOutput.close()\n self.gitError.close()\n+ self.gitStream = None\n \n def run(self, args):\n if self.importIntoRemotes:\n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"390715","messageId":"20200129111246.12196-3-luke@diamand.org","threadId":"52711","inReplyTo":"20200129111246.12196-2-luke@diamand.org","subject":"[PATCHv1 2/6] git-p4: add P4CommandException to report errors talking to Perforce","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T11:12:42Z","receivedAt":"2020-01-29T11:12:56Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Currently when there is a P4 error, git-p4 calls die() which just exits.\n\nThis then leaves the git-fast-import process still running, and can even\nleave p4 itself still running.\n\nAs a result, git-p4 fails to exit cleanly. This is a particular problem\nfor people running the unit tests in regression.\n\nUse this exception to report errors upwards, cleaning up as the error\npropagates.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 23724defe8..df2a956622 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -618,6 +618,14 @@ def __init__(self, exit_code, p4_result, limit):\n super(P4RequestSizeException, self).__init__(exit_code, p4_result)\n self.limit = limit\n \n+class P4CommandException(P4Exception):\n+ \"\"\" Something went wrong calling p4 which means we have to give up \"\"\"\n+ def __init__(self, msg):\n+ self.msg = msg\n+\n+ def __str__(self):\n+ return self.msg\n+\n def isModeExecChanged(src_mode, dst_mode):\n return isModeExec(src_mode) != isModeExec(dst_mode)\n \n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"390716","messageId":"20200129111246.12196-4-luke@diamand.org","threadId":"52711","inReplyTo":"20200129111246.12196-3-luke@diamand.org","subject":"[PATCHv1 3/6] git-p4: disable some pylint warnings, to get pylint output to something manageable","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T11:12:43Z","receivedAt":"2020-01-29T11:12:57Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"pylint is incredibly useful for finding bugs, but git-p4 has never used\nit, so there are a lot of warnings that while important, don't actually\nresult in bugs.\n\nLet's turn those off for now, so we can get some useful output.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex df2a956622..d796074b87 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -7,6 +7,14 @@\n # 2007 Trolltech ASA\n # License: MIT \n #\n+# pylint: disable=invalid-name,missing-docstring,too-many-arguments,broad-except\n+# pylint: disable=no-self-use,wrong-import-position,consider-iterating-dictionary\n+# pylint: disable=wrong-import-order,unused-import,too-few-public-methods\n+# pylint: disable=too-many-lines,ungrouped-imports,fixme,too-many-locals\n+# pylint: disable=line-too-long,bad-whitespace,superfluous-parens\n+# pylint: disable=too-many-statements,too-many-instance-attributes\n+# pylint: disable=too-many-branches,too-many-nested-blocks\n+#\n import sys\n if sys.hexversion < 0x02040000:\n # The limiter is the subprocess module\n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"390717","messageId":"20200129111246.12196-5-luke@diamand.org","threadId":"52711","inReplyTo":"20200129111246.12196-4-luke@diamand.org","subject":"[PATCHv1 4/6] git-p4: create helper function importRevisions()","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T11:12:44Z","receivedAt":"2020-01-29T11:12:59Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"This makes it easier to try/catch around this block of code to ensure\ncleanup following p4 failures is handled properly.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 132 ++++++++++++++++++++++++++++--------------------------\n 1 file changed, 68 insertions(+), 64 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex d796074b87..f90b43fe5e 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):\n print(\"IO error details: {}\".format(err))\n print(self.gitError.read())\n \n+\n+ def importRevisions(self, args, branch_arg_given):\n+ changes = []\n+\n+ if len(self.changesFile) > 0:\n+ output = open(self.changesFile).readlines()\n+ changeSet = set()\n+ for line in output:\n+ changeSet.add(int(line))\n+\n+ for change in changeSet:\n+ changes.append(change)\n+\n+ changes.sort()\n+ else:\n+ # catch \"git p4 sync\" with no new branches, in a repo that\n+ # does not have any existing p4 branches\n+ if len(args) == 0:\n+ if not self.p4BranchesInGit:\n+ die(\"No remote p4 branches. Perhaps you never did \\\"git p4 clone\\\" in here.\")\n+\n+ # The default branch is master, unless --branch is used to\n+ # specify something else. Make sure it exists, or complain\n+ # nicely about how to use --branch.\n+ if not self.detectBranches:\n+ if not branch_exists(self.branch):\n+ if branch_arg_given:\n+ die(\"Error: branch %s does not exist.\" % self.branch)\n+ else:\n+ die(\"Error: no branch %s; perhaps specify one with --branch.\" %\n+ self.branch)\n+\n+ if self.verbose:\n+ print(\"Getting p4 changes for %s...%s\" % (', '.join(self.depotPaths),\n+ self.changeRange))\n+ changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)\n+\n+ if len(self.maxChanges) > 0:\n+ changes = changes[:min(int(self.maxChanges), len(changes))]\n+\n+ if len(changes) == 0:\n+ if not self.silent:\n+ print(\"No changes to import!\")\n+ else:\n+ if not self.silent and not self.detectBranches:\n+ print(\"Import destination: %s\" % self.branch)\n+\n+ self.updatedBranches = set()\n+\n+ if not self.detectBranches:\n+ if args:\n+ # start a new branch\n+ self.initialParent = \"\"\n+ else:\n+ # build on a previous revision\n+ self.initialParent = parseRevision(self.branch)\n+\n+ self.importChanges(changes)\n+\n+ if not self.silent:\n+ print(\"\")\n+ if len(self.updatedBranches) > 0:\n+ sys.stdout.write(\"Updated branches: \")\n+ for b in self.updatedBranches:\n+ sys.stdout.write(\"%s \" % b)\n+ sys.stdout.write(\"\\n\")\n+\n def openStreams(self):\n self.importProcess = subprocess.Popen([\"git\", \"fast-import\"],\n stdin=subprocess.PIPE,\n@@ -3761,70 +3828,7 @@ def run(self, args):\n if revision:\n self.importHeadRevision(revision)\n else:\n- changes = []\n-\n- if len(self.changesFile) > 0:\n- output = open(self.changesFile).readlines()\n- changeSet = set()\n- for line in output:\n- changeSet.add(int(line))\n-\n- for change in changeSet:\n- changes.append(change)\n-\n- changes.sort()\n- else:\n- # catch \"git p4 sync\" with no new branches, in a repo that\n- # does not have any existing p4 branches\n- if len(args) == 0:\n- if not self.p4BranchesInGit:\n- die(\"No remote p4 branches. Perhaps you never did \\\"git p4 clone\\\" in here.\")\n-\n- # The default branch is master, unless --branch is used to\n- # specify something else. Make sure it exists, or complain\n- # nicely about how to use --branch.\n- if not self.detectBranches:\n- if not branch_exists(self.branch):\n- if branch_arg_given:\n- die(\"Error: branch %s does not exist.\" % self.branch)\n- else:\n- die(\"Error: no branch %s; perhaps specify one with --branch.\" %\n- self.branch)\n-\n- if self.verbose:\n- print(\"Getting p4 changes for %s...%s\" % (', '.join(self.depotPaths),\n- self.changeRange))\n- changes = p4ChangesForPaths(self.depotPaths, self.changeRange, self.changes_block_size)\n-\n- if len(self.maxChanges) > 0:\n- changes = changes[:min(int(self.maxChanges), len(changes))]\n-\n- if len(changes) == 0:\n- if not self.silent:\n- print(\"No changes to import!\")\n- else:\n- if not self.silent and not self.detectBranches:\n- print(\"Import destination: %s\" % self.branch)\n-\n- self.updatedBranches = set()\n-\n- if not self.detectBranches:\n- if args:\n- # start a new branch\n- self.initialParent = \"\"\n- else:\n- # build on a previous revision\n- self.initialParent = parseRevision(self.branch)\n-\n- self.importChanges(changes)\n-\n- if not self.silent:\n- print(\"\")\n- if len(self.updatedBranches) > 0:\n- sys.stdout.write(\"Updated branches: \")\n- for b in self.updatedBranches:\n- sys.stdout.write(\"%s \" % b)\n- sys.stdout.write(\"\\n\")\n+ self.importRevisions(args, branch_arg_given)\n \n if gitConfigBool(\"git-p4.importLabels\"):\n self.importLabels = True\n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"390718","messageId":"20200129111246.12196-6-luke@diamand.org","threadId":"52711","inReplyTo":"20200129111246.12196-5-luke@diamand.org","subject":"[PATCHv1 5/6] git-p4: cleanup better on error exit","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T11:12:45Z","receivedAt":"2020-01-29T11:13:00Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"After an error, git-p4 calls die(). This just exits, and leaves child\nprocesses still running.\n\nInstead of calling die(), raise an exception and catch it where the\nchild process(es) (git-fastimport) are created.\n\nThis was analyzed in detail here:\n\n https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/\n\nThis change does not address the particular issue of p4CmdList()\ninvoking a subchild and not waiting for it on error.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 43 ++++++++++++++++++++++++++++---------------\n 1 file changed, 28 insertions(+), 15 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex f90b43fe5e..a69a24bf4c 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -169,6 +169,9 @@ def calcDiskFree():\n return st.f_bavail * st.f_frsize\n \n def die(msg):\n+ \"\"\" Terminate execution. Make sure that any running child processes have been wait()ed for before\n+ calling this.\n+ \"\"\"\n if verbose:\n raise Exception(msg)\n else:\n@@ -3574,7 +3577,7 @@ def importRevisions(self, args, branch_arg_given):\n # does not have any existing p4 branches\n if len(args) == 0:\n if not self.p4BranchesInGit:\n- die(\"No remote p4 branches. Perhaps you never did \\\"git p4 clone\\\" in here.\")\n+ raise P4CommandException(\"No remote p4 branches. Perhaps you never did \\\"git p4 clone\\\" in here.\")\n \n # The default branch is master, unless --branch is used to\n # specify something else. Make sure it exists, or complain\n@@ -3582,9 +3585,9 @@ def importRevisions(self, args, branch_arg_given):\n if not self.detectBranches:\n if not branch_exists(self.branch):\n if branch_arg_given:\n- die(\"Error: branch %s does not exist.\" % self.branch)\n+ raise P4CommandException(\"Error: branch %s does not exist.\" % self.branch)\n else:\n- die(\"Error: no branch %s; perhaps specify one with --branch.\" %\n+ raise P4CommandException(\"Error: no branch %s; perhaps specify one with --branch.\" %\n self.branch)\n \n if self.verbose:\n@@ -3825,22 +3828,32 @@ def run(self, args):\n \n self.openStreams()\n \n- if revision:\n- self.importHeadRevision(revision)\n- else:\n- self.importRevisions(args, branch_arg_given)\n+ err = None\n \n- if gitConfigBool(\"git-p4.importLabels\"):\n- self.importLabels = True\n+ try:\n+ if revision:\n+ self.importHeadRevision(revision)\n+ else:\n+ self.importRevisions(args, branch_arg_given)\n \n- if self.importLabels:\n- p4Labels = getP4Labels(self.depotPaths)\n- gitTags = getGitTags()\n+ if gitConfigBool(\"git-p4.importLabels\"):\n+ self.importLabels = True\n \n- missingP4Labels = p4Labels - gitTags\n- self.importP4Labels(self.gitStream, missingP4Labels)\n+ if self.importLabels:\n+ p4Labels = getP4Labels(self.depotPaths)\n+ gitTags = getGitTags()\n \n- self.closeStreams()\n+ missingP4Labels = p4Labels - gitTags\n+ self.importP4Labels(self.gitStream, missingP4Labels)\n+\n+ except P4CommandException as e:\n+ err = e\n+\n+ finally:\n+ self.closeStreams()\n+\n+ if err:\n+ die(str(err))\n \n # Cleanup temporary branches created during import\n if self.tempBranches != []:\n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"390719","messageId":"20200129111246.12196-7-luke@diamand.org","threadId":"52711","inReplyTo":"20200129111246.12196-6-luke@diamand.org","subject":"[PATCHv1 6/6] git-p4: check for access to remote host earlier","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T11:12:46Z","receivedAt":"2020-01-29T11:13:01Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Check we can talk to the remote host before starting the git-fastimport\nsubchild.\n\nOtherwise we fail to connect, and then exit, leaving git-fastimport\nstill running since we did not wait() for it.\n\nSigned-off-by: Luke Diamand \n---\n git-p4.py | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex a69a24bf4c..eb5bc28cf9 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -3826,6 +3826,8 @@ def run(self, args):\n b = b[len(self.projectName):]\n self.createdBranches.add(b)\n \n+ p4_check_access()\n+\n self.openStreams()\n \n err = None\n-- \n2.20.1.390.gb5101f9297\n\n"},{"id":"390727","messageId":"CAPig+cRx3hG64nuDie69o_gdX39F=sR6D8LyA7J1rCErgu0aMA@mail.gmail.com","threadId":"52711","inReplyTo":"20200129111246.12196-5-luke@diamand.org","subject":"Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-29T15:00:29Z","receivedAt":"2020-01-29T15:00:44Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand wrote:\n> This makes it easier to try/catch around this block of code to ensure\n> cleanup following p4 failures is handled properly.\n>\n> Signed-off-by: Luke Diamand \n> ---\n> diff --git a/git-p4.py b/git-p4.py\n> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):\n> + def importRevisions(self, args, branch_arg_given):\n> + if len(self.changesFile) > 0:\n> + output = open(self.changesFile).readlines()\n\nNot a new problem (since this code is merely being relocated), but is\nthis leaking the open file? Should there be an accompanying close()\nsomewhere?\n\n f = open(self.changesFile)\n output = f.readlines()\n close(f)\n\nor something.\n"},{"id":"390734","messageId":"CAE5ih785Nv584RU-xEQSqrcuGiUU2zmjRrJ=ghtFX7_7EuiDnw@mail.gmail.com","threadId":"52711","inReplyTo":"CAPig+cRx3hG64nuDie69o_gdX39F=sR6D8LyA7J1rCErgu0aMA@mail.gmail.com","subject":"Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-29T18:36:38Z","receivedAt":"2020-01-29T18:36:53Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Wed, 29 Jan 2020 at 15:00, Eric Sunshine wrote:\n>\n> On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand wrote:\n> > This makes it easier to try/catch around this block of code to ensure\n> > cleanup following p4 failures is handled properly.\n> >\n> > Signed-off-by: Luke Diamand \n> > ---\n> > diff --git a/git-p4.py b/git-p4.py\n> > @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):\n> > + def importRevisions(self, args, branch_arg_given):\n> > + if len(self.changesFile) > 0:\n> > + output = open(self.changesFile).readlines()\n>\n> Not a new problem (since this code is merely being relocated), but is\n> this leaking the open file? Should there be an accompanying close()\n> somewhere?\n>\n> f = open(self.changesFile)\n> output = f.readlines()\n> close(f)\n\nwith open(self.changesFile) as f:\n output = f.readlines()\n\nI can put something like that in either as a followup, or the next re-roll.\n\n\n>\n> or something.\n"},{"id":"390782","messageId":"nycvar.QRO.7.76.6.2001301131280.46@tvgsbejvaqbjf.bet","threadId":"52711","inReplyTo":"20200129111246.12196-1-luke@diamand.org","subject":"Re: [PATCHv1 0/6] git-p4: wait() for child processes better","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T10:52:08Z","receivedAt":"2020-01-30T10:52:16Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Luke,\n\nOn Wed, 29 Jan 2020, Luke Diamand wrote:\n\n> git-p4 handles most errors by calling die(). This can leave child\n> processes still running, orphaned.\n>\n> https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/\n>\n> This is not a problem for humans, but for CI, it is.\n\nJust to make sure that we're talking about the same thing. Looking at\n\nhttps://dev.azure.com/git/git/_test/analytics?definitionId=11&contextType=build\n\nI am not even sure that `git-p4` is our biggest problem there. In that\nlist, the only flaky `git-p4` test I see is t9806.5. And it failed only\nonce recently:\nhttps://dev.azure.com/git/git/_build/results?buildId=1640&view=ms.vss-test-web.build-test-results-tab\n\nThe log looks like this:\n\n-- snip --\n[...]\nexpecting success of 9806.5 'sync when no master branch prints a nice error':\n\ttest_when_finished cleanup_git &&\n\tgit p4 clone --branch=refs/remotes/p4/sb --dest=\"$git\" //depot@2 &&\n\t(\n\t\tcd \"$git\" &&\n\t\ttest_must_fail git p4 sync 2>err &&\n\t\tgrep \"Error: no branch refs/remotes/p4/master\" err\n\t)\n\n+ test_when_finished cleanup_git\n+ test 0 = 0\n+ test_cleanup={ cleanup_git\n\t\t} && (exit \"$eval_ret\"); eval_ret=$?; :\n+ git p4 clone --branch=refs/remotes/p4/sb --dest=/home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git //depot@2\nInitialized empty Git repository in /home/vsts/work/1/s/t/trash\ndirectory.t9806-git-p4-options/git/.git/\nPerforce db files in '.' will be created if missing...\nPerforce db files in '.' will be created if missing...\nPerforce db files in '.' will be created if missing...\nPerforce db files in '.' will be created if missing...\nPerforce db files in '.' will be created if missing...\nImporting from //depot@2 into /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\nDoing initial import of //depot/ from revision @2 into refs/remotes/p4/sb\n+ cd /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n+ test_must_fail git p4 sync\n+ _test_ok=\n+ git p4 sync\nPerforming incremental import into refs/remotes/p4/master git branch\nDepot paths: //depot/\n+ exit_code=1\n+ test 1 -eq 0\n+ test_match_signal 13 1\n+ test 1 = 141\n+ test 1 = 269\n+ return 1\n+ test 1 -gt 129\n+ test 1 -eq 127\n+ test 1 -eq 126\n+ return 0\n+ grep Error: no branch refs/remotes/p4/master err\nError: no branch refs/remotes/p4/master; perhaps specify one with --branch.\n+ cleanup_git\n+ retry_until_success rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n+ nr_tries_left=60\n+ rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n+ test_path_is_missing /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n+ test -e /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n+ echo Path exists:\nPath exists:\n+ ls -ld /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\ndrwxr-xr-x 3 vsts docker 4096 Jan 24 22:20 /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n+ test 1 -ge 1\n+ echo /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n+ false\n+ eval_ret=1\n+ :\n-- snap --\n\nFWIW I see the same test being flaky in Git for Windows (but it _also_\nonly happened once in the past 14 days):\nhttps://dev.azure.com/git-for-windows/git/_test/analytics?definitionId=17&contextType=build\nand\nhttps://dev.azure.com/git-for-windows/git/_build/results?buildId=49174&view=ms.vss-test-web.build-test-results-tab\n\nThe log suggests to me that there is a path that has not been cleaned up,\nand _maybe_ it is timing-related, but it is also possible that a child\nprocess is still running that should have cleaned it up.\n\nAre your patches about this failure?\n\nI see that PATCH 5/6 talks about Gábor's analysis of t9800.6 in\nhttps://public-inbox.org/git/20190227094926.GE19739@szeder.dev/ which\n_looks_ similar.\n\nFWIW I looked over your patches and they seem relatively obvious, even for\nsomebody like me who barely gets to code in Python anymore.\n\nThanks,\nDscho\n\n>\n> This change improves things by raising an exception and cleaning up\n> further up the stack, rather than simply calling die().\n>\n> This is only done in a few places, such that the tests pass with the changes\n> suggested in the link (adding sleep strategically) but there are still\n> plenty of places where git-p4 calls die().\n>\n> This also adds some pylint disables, so that we can start to run pylint\n> on git-p4.\n>\n> Luke Diamand (6):\n> git-p4: make closeStreams() idempotent\n> git-p4: add P4CommandException to report errors talking to Perforce\n> git-p4: disable some pylint warnings, to get pylint output to\n> something manageable\n> git-p4: create helper function importRevisions()\n> git-p4: cleanup better on error exit\n> git-p4: check for access to remote host earlier\n>\n> git-p4.py | 180 +++++++++++++++++++++++++++++++++---------------------\n> 1 file changed, 109 insertions(+), 71 deletions(-)\n>\n> --\n> 2.20.1.390.gb5101f9297\n>\n>\n"},{"id":"390785","messageId":"CAE5ih7_Rh7L=nOfWEY9FhFYX4swHqS0LqTLG=7L_K363__gJkA@mail.gmail.com","threadId":"52711","inReplyTo":"nycvar.QRO.7.76.6.2001301131280.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCHv1 0/6] git-p4: wait() for child processes better","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-30T11:33:57Z","receivedAt":"2020-01-30T11:34:12Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Thu, 30 Jan 2020 at 10:52, Johannes Schindelin\n wrote:\n>\n> Hi Luke,\n>\n> On Wed, 29 Jan 2020, Luke Diamand wrote:\n>\n> > git-p4 handles most errors by calling die(). This can leave child\n> > processes still running, orphaned.\n> >\n> > https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/\n> >\n> > This is not a problem for humans, but for CI, it is.\n>\n> Just to make sure that we're talking about the same thing. Looking at\n>\n> https://dev.azure.com/git/git/_test/analytics?definitionId=11&contextType=build\n>\n> I am not even sure that `git-p4` is our biggest problem there. In that\n> list, the only flaky `git-p4` test I see is t9806.5. And it failed only\n> once recently:\n> https://dev.azure.com/git/git/_build/results?buildId=1640&view=ms.vss-test-web.build-test-results-tab\n>\n> The log looks like this:\n>\n> -- snip --\n> [...]\n> expecting success of 9806.5 'sync when no master branch prints a nice error':\n> test_when_finished cleanup_git &&\n> git p4 clone --branch=refs/remotes/p4/sb --dest=\"$git\" //depot@2 &&\n> (\n> cd \"$git\" &&\n> test_must_fail git p4 sync 2>err &&\n> grep \"Error: no branch refs/remotes/p4/master\" err\n> )\n>\n> + test_when_finished cleanup_git\n> + test 0 = 0\n> + test_cleanup={ cleanup_git\n> } && (exit \"$eval_ret\"); eval_ret=$?; :\n> + git p4 clone --branch=refs/remotes/p4/sb --dest=/home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git //depot@2\n> Initialized empty Git repository in /home/vsts/work/1/s/t/trash\n> directory.t9806-git-p4-options/git/.git/\n> Perforce db files in '.' will be created if missing...\n> Perforce db files in '.' will be created if missing...\n> Perforce db files in '.' will be created if missing...\n> Perforce db files in '.' will be created if missing...\n> Perforce db files in '.' will be created if missing...\n> Importing from //depot@2 into /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> Doing initial import of //depot/ from revision @2 into refs/remotes/p4/sb\n> + cd /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> + test_must_fail git p4 sync\n> + _test_ok=\n> + git p4 sync\n> Performing incremental import into refs/remotes/p4/master git branch\n> Depot paths: //depot/\n> + exit_code=1\n> + test 1 -eq 0\n> + test_match_signal 13 1\n> + test 1 = 141\n> + test 1 = 269\n> + return 1\n> + test 1 -gt 129\n> + test 1 -eq 127\n> + test 1 -eq 126\n> + return 0\n> + grep Error: no branch refs/remotes/p4/master err\n> Error: no branch refs/remotes/p4/master; perhaps specify one with --branch.\n> + cleanup_git\n> + retry_until_success rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> + nr_tries_left=60\n> + rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> + test_path_is_missing /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> + test -e /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> + echo Path exists:\n> Path exists:\n> + ls -ld /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> drwxr-xr-x 3 vsts docker 4096 Jan 24 22:20 /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> + test 1 -ge 1\n> + echo /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git\n> + false\n> + eval_ret=1\n> + :\n> -- snap --\n\nThis should be fixed by my change. When that error occurs, the current\nversion calls die(), with my change it raises an exception, and then\ncleans up further up the stack.\n\n- die(\"Error: no branch %s; perhaps specify\none with --branch.\" %\n+ raise P4CommandException(\"Error: no\nbranch %s; perhaps specify one with --branch.\" %\n\n\n>\n> FWIW I see the same test being flaky in Git for Windows (but it _also_\n> only happened once in the past 14 days):\n> https://dev.azure.com/git-for-windows/git/_test/analytics?definitionId=17&contextType=build\n> and\n> https://dev.azure.com/git-for-windows/git/_build/results?buildId=49174&view=ms.vss-test-web.build-test-results-tab\n>\n> The log suggests to me that there is a path that has not been cleaned up,\n> and _maybe_ it is timing-related, but it is also possible that a child\n> process is still running that should have cleaned it up.\n>\n> Are your patches about this failure?\n\nI was actually looking at Gábor's report. He has a nifty hack which\nmakes the errors crop up very rapidly, so I just fixed all of the\nfailures that arose from that.\n\nIt's easy to see from inspecting the code that there are plenty of\nother places where this is not handled properly. Probably those should\nwait until the python3 fixes have been merged and/or there is some\ndemand for them.\n\n>\n> I see that PATCH 5/6 talks about Gábor's analysis of t9800.6 in\n> https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/ which\n> _looks_ similar.\n>\n> FWIW I looked over your patches and they seem relatively obvious, even for\n> somebody like me who barely gets to code in Python anymore.\n\nThanks!\nLuke\n\n>\n> Thanks,\n> Dscho\n>\n> >\n> > This change improves things by raising an exception and cleaning up\n> > further up the stack, rather than simply calling die().\n> >\n> > This is only done in a few places, such that the tests pass with the changes\n> > suggested in the link (adding sleep strategically) but there are still\n> > plenty of places where git-p4 calls die().\n> >\n> > This also adds some pylint disables, so that we can start to run pylint\n> > on git-p4.\n> >\n> > Luke Diamand (6):\n> > git-p4: make closeStreams() idempotent\n> > git-p4: add P4CommandException to report errors talking to Perforce\n> > git-p4: disable some pylint warnings, to get pylint output to\n> > something manageable\n> > git-p4: create helper function importRevisions()\n> > git-p4: cleanup better on error exit\n> > git-p4: check for access to remote host earlier\n> >\n> > git-p4.py | 180 +++++++++++++++++++++++++++++++++---------------------\n> > 1 file changed, 109 insertions(+), 71 deletions(-)\n> >\n> > --\n> > 2.20.1.390.gb5101f9297\n> >\n> >\n"},{"id":"390832","messageId":"xmqqzhe4btr0.fsf@gitster-ct.c.googlers.com","threadId":"52711","inReplyTo":"CAPig+cRx3hG64nuDie69o_gdX39F=sR6D8LyA7J1rCErgu0aMA@mail.gmail.com","subject":"Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-30T19:59:31Z","receivedAt":"2020-01-30T19:59:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand wrote:\n>> This makes it easier to try/catch around this block of code to ensure\n>> cleanup following p4 failures is handled properly.\n>>\n>> Signed-off-by: Luke Diamand \n>> ---\n>> diff --git a/git-p4.py b/git-p4.py\n>> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):\n>> + def importRevisions(self, args, branch_arg_given):\n>> + if len(self.changesFile) > 0:\n>> + output = open(self.changesFile).readlines()\n>\n> Not a new problem (since this code is merely being relocated), but is\n> this leaking the open file? Should there be an accompanying close()\n> somewhere?\n>\n> f = open(self.changesFile)\n> output = f.readlines()\n> close(f)\n>\n> or something.\n\nHmph, I was naively hoping that the (never assigned to any variable)\nlast reference going away at the end of the statement would make the\nfile object dead, and we can let eventual GC to close it.\n\nNevertheless it would not hurt to explicitly control the lifetime.\n"},{"id":"390897","messageId":"CAE5ih78VwT3iRb8pmyN9g-JnU2dET2ZZ4X8CHWSxdARtq3-tEg@mail.gmail.com","threadId":"52711","inReplyTo":"xmqqzhe4btr0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2020-01-31T10:36:48Z","receivedAt":"2020-01-31T10:37:03Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On Thu, 30 Jan 2020 at 19:59, Junio C Hamano wrote:\n>\n> Eric Sunshine writes:\n>\n> > On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand wrote:\n> >> This makes it easier to try/catch around this block of code to ensure\n> >> cleanup following p4 failures is handled properly.\n> >>\n> >> Signed-off-by: Luke Diamand \n> >> ---\n> >> diff --git a/git-p4.py b/git-p4.py\n> >> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):\n> >> + def importRevisions(self, args, branch_arg_given):\n> >> + if len(self.changesFile) > 0:\n> >> + output = open(self.changesFile).readlines()\n> >\n> > Not a new problem (since this code is merely being relocated), but is\n> > this leaking the open file? Should there be an accompanying close()\n> > somewhere?\n> >\n> > f = open(self.changesFile)\n> > output = f.readlines()\n> > close(f)\n> >\n> > or something.\n>\n> Hmph, I was naively hoping that the (never assigned to any variable)\n> last reference going away at the end of the statement would make the\n> file object dead, and we can let eventual GC to close it.\n>\n> Nevertheless it would not hurt to explicitly control the lifetime.\n\nYou are right, there is no file descriptor leak. It's easy enough to\nwrite some test code to demonstrate this.\n"}]}