[PATCHv1 0/6] git-p4: wait() for child processes better

[PATCHv1 0/6] git-p4: wait() for child processes better

[Luke Diamand]

git-p4 handles most errors by calling die(). This can leave child
processes still running, orphaned.

    https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/

This is not a problem for humans, but for CI, it is.

This change improves things by raising an exception and cleaning up
further up the stack, rather than simply calling die().

This is only done in a few places, such that the tests pass with the changes
suggested in the link (adding sleep strategically) but there are still
plenty of places where git-p4 calls die().

This also adds some pylint disables, so that we can start to run pylint
on git-p4.

Luke Diamand (6):
  git-p4: make closeStreams() idempotent
  git-p4: add P4CommandException to report errors talking to Perforce
  git-p4: disable some pylint warnings, to get pylint output to
    something manageable
  git-p4: create helper function importRevisions()
  git-p4: cleanup better on error exit
  git-p4: check for access to remote host earlier

 git-p4.py | 180 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 109 insertions(+), 71 deletions(-)

-- 
2.20.1.390.gb5101f9297

[PATCHv1 1/6] git-p4: make closeStreams() idempotent

[Luke Diamand]

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 <luke@diamand.org>
---
 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 @@ def openStreams(self):
         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:
-- 
2.20.1.390.gb5101f9297

[PATCHv1 2/6] git-p4: add P4CommandException to report errors talking to Perforce

[Luke Diamand]

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 <luke@diamand.org>
---
 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 @@ def __init__(self, exit_code, p4_result, limit):
         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)
 
-- 
2.20.1.390.gb5101f9297

[PATCHv1 3/6] git-p4: disable some pylint warnings, to get pylint output to something manageable

[Luke Diamand]

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 <luke@diamand.org>
---
 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 <http://www.opensource.org/licenses/mit-license.php>
 #
+# 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
-- 
2.20.1.390.gb5101f9297

[PATCHv1 4/6] git-p4: create helper function importRevisions()

[Luke Diamand]

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 <luke@diamand.org>
---
 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 @@ def importHeadRevision(self, revision):
             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 @@ def run(self, args):
         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
-- 
2.20.1.390.gb5101f9297

[PATCHv1 5/6] git-p4: cleanup better on error exit

[Luke Diamand]

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 <luke@diamand.org>
---
 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 @@ def importRevisions(self, args, branch_arg_given):
             # 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 @@ def importRevisions(self, args, branch_arg_given):
                 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 @@ def run(self, args):
 
         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 != []:
-- 
2.20.1.390.gb5101f9297

[PATCHv1 6/6] git-p4: check for access to remote host earlier

[Luke Diamand]

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 <luke@diamand.org>
---
 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 @@ def run(self, args):
                     b = b[len(self.projectName):]
                 self.createdBranches.add(b)
 
+        p4_check_access()
+
         self.openStreams()
 
         err = None
-- 
2.20.1.390.gb5101f9297

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

[Eric Sunshine]

On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
> 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 <luke@diamand.org>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
> +    def importRevisions(self, args, branch_arg_given):
> +        if len(self.changesFile) > 0:
> +            output = open(self.changesFile).readlines()

Not a new problem (since this code is merely being relocated), but is
this leaking the open file? Should there be an accompanying close()
somewhere?

    f = open(self.changesFile)
    output = f.readlines()
    close(f)

or something.

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

[Luke Diamand]

On Wed, 29 Jan 2020 at 15:00, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
> > 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 <luke@diamand.org>
> > ---
> > diff --git a/git-p4.py b/git-p4.py
> > @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
> > +    def importRevisions(self, args, branch_arg_given):
> > +        if len(self.changesFile) > 0:
> > +            output = open(self.changesFile).readlines()
>
> Not a new problem (since this code is merely being relocated), but is
> this leaking the open file? Should there be an accompanying close()
> somewhere?
>
>     f = open(self.changesFile)
>     output = f.readlines()
>     close(f)

with open(self.changesFile) as f:
    output = f.readlines()

I can put something like that in either as a followup, or the next re-roll.


>
> or something.

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
>> 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 <luke@diamand.org>
>> ---
>> diff --git a/git-p4.py b/git-p4.py
>> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
>> +    def importRevisions(self, args, branch_arg_given):
>> +        if len(self.changesFile) > 0:
>> +            output = open(self.changesFile).readlines()
>
> Not a new problem (since this code is merely being relocated), but is
> this leaking the open file? Should there be an accompanying close()
> somewhere?
>
>     f = open(self.changesFile)
>     output = f.readlines()
>     close(f)
>
> or something.

Hmph, I was naively hoping that the (never assigned to any variable)
last reference going away at the end of the statement would make the
file object dead, and we can let eventual GC to close it.

Nevertheless it would not hurt to explicitly control the lifetime.

Re: [PATCHv1 4/6] git-p4: create helper function importRevisions()

[Luke Diamand]

On Thu, 30 Jan 2020 at 19:59, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > On Wed, Jan 29, 2020 at 6:13 AM Luke Diamand <luke@diamand.org> wrote:
> >> 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 <luke@diamand.org>
> >> ---
> >> diff --git a/git-p4.py b/git-p4.py
> >> @@ -3555,6 +3555,73 @@ def importHeadRevision(self, revision):
> >> +    def importRevisions(self, args, branch_arg_given):
> >> +        if len(self.changesFile) > 0:
> >> +            output = open(self.changesFile).readlines()
> >
> > Not a new problem (since this code is merely being relocated), but is
> > this leaking the open file? Should there be an accompanying close()
> > somewhere?
> >
> >     f = open(self.changesFile)
> >     output = f.readlines()
> >     close(f)
> >
> > or something.
>
> Hmph, I was naively hoping that the (never assigned to any variable)
> last reference going away at the end of the statement would make the
> file object dead, and we can let eventual GC to close it.
>
> Nevertheless it would not hurt to explicitly control the lifetime.

You are right, there is no file descriptor leak. It's easy enough to
write some test code to demonstrate this.

Re: [PATCHv1 0/6] git-p4: wait() for child processes better

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5033 bytes --]

Hi Luke,

On Wed, 29 Jan 2020, Luke Diamand wrote:

> git-p4 handles most errors by calling die(). This can leave child
> processes still running, orphaned.
>
>     https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/
>
> This is not a problem for humans, but for CI, it is.

Just to make sure that we're talking about the same thing. Looking at

https://dev.azure.com/git/git/_test/analytics?definitionId=11&contextType=build

I am not even sure that `git-p4` is our biggest problem there. In that
list, the only flaky `git-p4` test I see is t9806.5. And it failed only
once recently:
https://dev.azure.com/git/git/_build/results?buildId=1640&view=ms.vss-test-web.build-test-results-tab

The log looks like this:

-- snip --
[...]
expecting success of 9806.5 'sync when no master branch prints a nice error':
	test_when_finished cleanup_git &&
	git p4 clone --branch=refs/remotes/p4/sb --dest="$git" //depot@2 &&
	(
		cd "$git" &&
		test_must_fail git p4 sync 2>err &&
		grep "Error: no branch refs/remotes/p4/master" err
	)

+ test_when_finished cleanup_git
+ test 0 = 0
+ test_cleanup={ cleanup_git
		} && (exit "$eval_ret"); eval_ret=$?; :
+ git p4 clone --branch=refs/remotes/p4/sb --dest=/home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git //depot@2
Initialized empty Git repository in /home/vsts/work/1/s/t/trash
directory.t9806-git-p4-options/git/.git/
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Perforce db files in '.' will be created if missing...
Importing from //depot@2 into /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
Doing initial import of //depot/ from revision @2 into refs/remotes/p4/sb
+ cd /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test_must_fail git p4 sync
+ _test_ok=
+ git p4 sync
Performing incremental import into refs/remotes/p4/master git branch
Depot paths: //depot/
+ exit_code=1
+ test 1 -eq 0
+ test_match_signal 13 1
+ test 1 = 141
+ test 1 = 269
+ return 1
+ test 1 -gt 129
+ test 1 -eq 127
+ test 1 -eq 126
+ return 0
+ grep Error: no branch refs/remotes/p4/master err
Error: no branch refs/remotes/p4/master; perhaps specify one with --branch.
+ cleanup_git
+ retry_until_success rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ nr_tries_left=60
+ rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test_path_is_missing /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test -e /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ echo Path exists:
Path exists:
+ ls -ld /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
drwxr-xr-x 3 vsts docker 4096 Jan 24 22:20 /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
+ test 1 -ge 1
+ 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
+ false
+ eval_ret=1
+ :
-- snap --

FWIW I see the same test being flaky in Git for Windows (but it _also_
only happened once in the past 14 days):
https://dev.azure.com/git-for-windows/git/_test/analytics?definitionId=17&contextType=build
and
https://dev.azure.com/git-for-windows/git/_build/results?buildId=49174&view=ms.vss-test-web.build-test-results-tab

The log suggests to me that there is a path that has not been cleaned up,
and _maybe_ it is timing-related, but it is also possible that a child
process is still running that should have cleaned it up.

Are your patches about this failure?

I see that PATCH 5/6 talks about Gábor's analysis of t9800.6 in
https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/ which
_looks_ similar.

FWIW I looked over your patches and they seem relatively obvious, even for
somebody like me who barely gets to code in Python anymore.

Thanks,
Dscho

>
> This change improves things by raising an exception and cleaning up
> further up the stack, rather than simply calling die().
>
> This is only done in a few places, such that the tests pass with the changes
> suggested in the link (adding sleep strategically) but there are still
> plenty of places where git-p4 calls die().
>
> This also adds some pylint disables, so that we can start to run pylint
> on git-p4.
>
> Luke Diamand (6):
>   git-p4: make closeStreams() idempotent
>   git-p4: add P4CommandException to report errors talking to Perforce
>   git-p4: disable some pylint warnings, to get pylint output to
>     something manageable
>   git-p4: create helper function importRevisions()
>   git-p4: cleanup better on error exit
>   git-p4: check for access to remote host earlier
>
>  git-p4.py | 180 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 109 insertions(+), 71 deletions(-)
>
> --
> 2.20.1.390.gb5101f9297
>
>

Re: [PATCHv1 0/6] git-p4: wait() for child processes better

[Luke Diamand]

On Thu, 30 Jan 2020 at 10:52, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Luke,
>
> On Wed, 29 Jan 2020, Luke Diamand wrote:
>
> > git-p4 handles most errors by calling die(). This can leave child
> > processes still running, orphaned.
> >
> >     https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/
> >
> > This is not a problem for humans, but for CI, it is.
>
> Just to make sure that we're talking about the same thing. Looking at
>
> https://dev.azure.com/git/git/_test/analytics?definitionId=11&contextType=build
>
> I am not even sure that `git-p4` is our biggest problem there. In that
> list, the only flaky `git-p4` test I see is t9806.5. And it failed only
> once recently:
> https://dev.azure.com/git/git/_build/results?buildId=1640&view=ms.vss-test-web.build-test-results-tab
>
> The log looks like this:
>
> -- snip --
> [...]
> expecting success of 9806.5 'sync when no master branch prints a nice error':
>         test_when_finished cleanup_git &&
>         git p4 clone --branch=refs/remotes/p4/sb --dest="$git" //depot@2 &&
>         (
>                 cd "$git" &&
>                 test_must_fail git p4 sync 2>err &&
>                 grep "Error: no branch refs/remotes/p4/master" err
>         )
>
> + test_when_finished cleanup_git
> + test 0 = 0
> + test_cleanup={ cleanup_git
>                 } && (exit "$eval_ret"); eval_ret=$?; :
> + git p4 clone --branch=refs/remotes/p4/sb --dest=/home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git //depot@2
> Initialized empty Git repository in /home/vsts/work/1/s/t/trash
> directory.t9806-git-p4-options/git/.git/
> Perforce db files in '.' will be created if missing...
> Perforce db files in '.' will be created if missing...
> Perforce db files in '.' will be created if missing...
> Perforce db files in '.' will be created if missing...
> Perforce db files in '.' will be created if missing...
> Importing from //depot@2 into /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> Doing initial import of //depot/ from revision @2 into refs/remotes/p4/sb
> + cd /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> + test_must_fail git p4 sync
> + _test_ok=
> + git p4 sync
> Performing incremental import into refs/remotes/p4/master git branch
> Depot paths: //depot/
> + exit_code=1
> + test 1 -eq 0
> + test_match_signal 13 1
> + test 1 = 141
> + test 1 = 269
> + return 1
> + test 1 -gt 129
> + test 1 -eq 127
> + test 1 -eq 126
> + return 0
> + grep Error: no branch refs/remotes/p4/master err
> Error: no branch refs/remotes/p4/master; perhaps specify one with --branch.
> + cleanup_git
> + retry_until_success rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> + nr_tries_left=60
> + rm -r /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> + test_path_is_missing /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> + test -e /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> + echo Path exists:
> Path exists:
> + ls -ld /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> drwxr-xr-x 3 vsts docker 4096 Jan 24 22:20 /home/vsts/work/1/s/t/trash directory.t9806-git-p4-options/git
> + test 1 -ge 1
> + 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
> + false
> + eval_ret=1
> + :
> -- snap --

This should be fixed by my change. When that error occurs, the current
version calls die(), with my change it raises an exception, and then
cleans up further up the stack.

-                            die("Error: no branch %s; perhaps specify
one with --branch." %
+                            raise P4CommandException("Error: no
branch %s; perhaps specify one with --branch." %


>
> FWIW I see the same test being flaky in Git for Windows (but it _also_
> only happened once in the past 14 days):
> https://dev.azure.com/git-for-windows/git/_test/analytics?definitionId=17&contextType=build
> and
> https://dev.azure.com/git-for-windows/git/_build/results?buildId=49174&view=ms.vss-test-web.build-test-results-tab
>
> The log suggests to me that there is a path that has not been cleaned up,
> and _maybe_ it is timing-related, but it is also possible that a child
> process is still running that should have cleaned it up.
>
> Are your patches about this failure?

I was actually looking at Gábor's report. He has a nifty hack which
makes the errors crop up very rapidly, so I just fixed all of the
failures that arose from that.

It's easy to see from inspecting the code that there are plenty of
other places where this is not handled properly. Probably those should
wait until the python3 fixes have been merged and/or there is some
demand for them.

>
> I see that PATCH 5/6 talks about Gábor's analysis of t9800.6 in
> https://public-inbox.org/git/20190227094926.GE19739@szeder.dev/ which
> _looks_ similar.
>
> FWIW I looked over your patches and they seem relatively obvious, even for
> somebody like me who barely gets to code in Python anymore.

Thanks!
Luke

>
> Thanks,
> Dscho
>
> >
> > This change improves things by raising an exception and cleaning up
> > further up the stack, rather than simply calling die().
> >
> > This is only done in a few places, such that the tests pass with the changes
> > suggested in the link (adding sleep strategically) but there are still
> > plenty of places where git-p4 calls die().
> >
> > This also adds some pylint disables, so that we can start to run pylint
> > on git-p4.
> >
> > Luke Diamand (6):
> >   git-p4: make closeStreams() idempotent
> >   git-p4: add P4CommandException to report errors talking to Perforce
> >   git-p4: disable some pylint warnings, to get pylint output to
> >     something manageable
> >   git-p4: create helper function importRevisions()
> >   git-p4: cleanup better on error exit
> >   git-p4: check for access to remote host earlier
> >
> >  git-p4.py | 180 +++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 109 insertions(+), 71 deletions(-)
> >
> > --
> > 2.20.1.390.gb5101f9297
> >
> >