| Message ID | 20200129111246.12196-5-luke@diamand.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-p4: wait() for child processes better | expand |
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.
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.
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.
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.
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
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(-)