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

[Luke Diamand <luke@diamand.org>]

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.