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

Message ID 20200129111246.12196-5-luke@diamand.org
State New
Headers show
Series
  • git-p4: wait() for child processes better
Related show

Commit Message

Luke Diamand Jan. 29, 2020, 11:12 a.m. UTC
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(-)

Comments

Eric Sunshine Jan. 29, 2020, 3 p.m. UTC | #1
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.
Luke Diamand Jan. 29, 2020, 6:36 p.m. UTC | #2
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.
Junio C Hamano Jan. 30, 2020, 7:59 p.m. UTC | #3
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.
Luke Diamand Jan. 31, 2020, 10:36 a.m. UTC | #4
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.

Patch
diff mbox series

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