[PATCH 0/1] git-p4: auto-delete named temporary file

[PATCH 0/1] git-p4: auto-delete named temporary file

[Philip McGraw via GitGitGadget]

Take new approach using the NamedTemporaryFile() file-like object as input
to the ZipFile() which auto-deletes after implicit close leaving with scope.

Original code produced double-open problems on Windows platform from using
already open NamedTemporaryFile() generated filename instead of object.

Thanks to Andrey for patiently suggesting several iterations on this change
for avoiding exceptions!

Also print error details after resulting IOError to make debugging cause of
exception less mysterious when it has nothing to do with "git version recent
enough."

Signed-off-by: Philip.McGraw Philip.McGraw@bentley.com
[Philip.McGraw@bentley.com]

Philip.McGraw (1):
  git-p4: auto-delete named temporary file

 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)


base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-303%2Fphilip-mcgraw%2Fgit-p4-auto-delete-named-temporary-file-v3-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-303/philip-mcgraw/git-p4-auto-delete-named-temporary-file-v3-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/303
-- 
gitgitgadget

Re: [PATCH 0/1] git-p4: auto-delete named temporary file

[Andrey]

01.08.2019, 17:39, "Philip McGraw via GitGitGadget" <gitgitgadget@gmail.com>:
> Take new approach using the NamedTemporaryFile() file-like object as input
> to the ZipFile() which auto-deletes after implicit close leaving with scope.
>
> Original code produced double-open problems on Windows platform from using
> already open NamedTemporaryFile() generated filename instead of object.
>
> Thanks to Andrey for patiently suggesting several iterations on this change
> for avoiding exceptions!
>
> Also print error details after resulting IOError to make debugging cause of
> exception less mysterious when it has nothing to do with "git version recent
> enough."
>
> Signed-off-by: Philip.McGraw Philip.McGraw@bentley.com
> [Philip.McGraw@bentley.com]
>
> Philip.McGraw (1):
>   git-p4: auto-delete named temporary file
>
>  git-p4.py | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-303%2Fphilip-mcgraw%2Fgit-p4-auto-delete-named-temporary-file-v3-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-303/philip-mcgraw/git-p4-auto-delete-named-temporary-file-v3-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/303
> --
> gitgitgadget

Looks good to me!
Reviewed-by: Andrey Mazo <ahippo@yandex.com>


-- 
Andrey.

[PATCH v2 0/1] git-p4: auto-delete named temporary file

[Philip McGraw via GitGitGadget]

Take new approach using the NamedTemporaryFile() file-like object as input
to the ZipFile() which auto-deletes after implicit close leaving with scope.

Original code produced double-open problems on Windows platform from using
already open NamedTemporaryFile() generated filename instead of object.

Thanks to Andrey for patiently suggesting several iterations on this change
for avoiding exceptions!

Also print error details after resulting IOError to make debugging cause of
exception less mysterious when it has nothing to do with "git version recent
enough."

Signed-off-by: Philip.McGraw Philip.McGraw@bentley.com
[Philip.McGraw@bentley.com]

Philip.McGraw (1):
  git-p4: auto-delete named temporary file

 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)


base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-303%2Fphilip-mcgraw%2Fgit-p4-auto-delete-named-temporary-file-v3-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-303/philip-mcgraw/git-p4-auto-delete-named-temporary-file-v3-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/303

Range-diff vs v1:

 1:  1b270ef9a6 ! 1:  7e59b5cec2 git-p4: auto-delete named temporary file
     @@ -2,13 +2,17 @@
      
          git-p4: auto-delete named temporary file
      
     +    Avoid double-open exceptions on Windows platform when
     +    calculating for lfs compressed size threshold
     +    (git-p4.largeFileCompressedThreshold) comparisons.
     +
          Take new approach using the NamedTemporaryFile()
          file-like object as input to the ZipFile() which
          auto-deletes after implicit close leaving with scope.
      
     -    Original code produced double-open problems on Windows
     -    platform from using already open NamedTemporaryFile()
     -    generated filename instead of object.
     +    Original code had double-open exception on Windows
     +    platform because file still open from NamedTemporaryFile()
     +    using generated filename instead of object.
      
          Thanks to Andrey for patiently suggesting several
          iterations on this change for avoiding exceptions!
     @@ -18,6 +22,7 @@
          nothing to do with "git version recent enough."
      
          Signed-off-by: Philip.McGraw <Philip.McGraw@bentley.com>
     +    Reviewed-by: Andrey Mazo <ahippo@yandex.com>
      
       diff --git a/git-p4.py b/git-p4.py
       --- a/git-p4.py

-- 
gitgitgadget

[PATCH v2 1/1] git-p4: auto-delete named temporary file

[Andrey Mazo]

From: "Philip.McGraw" <Philip.McGraw@bentley.com>

Avoid double-open exceptions on Windows platform when
calculating for lfs compressed size threshold
(git-p4.largeFileCompressedThreshold) comparisons.

Take new approach using the NamedTemporaryFile()
file-like object as input to the ZipFile() which
auto-deletes after implicit close leaving with scope.

Original code had double-open exception on Windows
platform because file still open from NamedTemporaryFile()
using generated filename instead of object.

Thanks to Andrey for patiently suggesting several
iterations on this change for avoiding exceptions!

Also print error details after resulting IOError to make
debugging cause of exception less mysterious when it has
nothing to do with "git version recent enough."

Signed-off-by: Philip.McGraw <Philip.McGraw@bentley.com>
Reviewed-by: Andrey Mazo <ahippo@yandex.com>
---
 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index c71a6832e2..33bdb14fd1 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1158,17 +1158,15 @@ def exceedsLargeFileThreshold(self, relPath, contents):
         if gitConfigInt('git-p4.largeFileCompressedThreshold'):
             contentsSize = sum(len(d) for d in contents)
             if contentsSize <= gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return False
             contentTempFile = self.generateTempFile(contents)
-            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
-            zf.close()
-            compressedContentsSize = zf.infolist()[0].compress_size
+            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
+            with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
+                zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
+                compressedContentsSize = zf.infolist()[0].compress_size
             os.remove(contentTempFile)
-            os.remove(compressedContentFile.name)
             if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return True
         return False
 
     def addLargeFile(self, relPath):
@@ -3512,12 +3510,13 @@ def importHeadRevision(self, revision):
         details["time"] = res["time"]
 
         self.updateOptionDict(details)
         try:
             self.commit(details, self.extractFilesFromCommit(details), self.branch)
-        except IOError:
+        except IOError as err:
             print("IO error with git fast-import. Is your git version recent enough?")
+            print("IO error details: {}".format(err))
             print(self.gitError.read())
 
     def openStreams(self):
         self.importProcess = subprocess.Popen(["git", "fast-import"],
                                               stdin=subprocess.PIPE,

base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
Pull-Request: https://github.com/gitgitgadget/git/pull/303
-- 
2.21.0

Re: [PATCH v2 1/1] git-p4: auto-delete named temporary file

[Junio C Hamano]

Andrey Mazo <ahippo@yandex.ru> writes:

> From: "Philip.McGraw" <Philip.McGraw@bentley.com>
>
> Avoid double-open exceptions on Windows platform when
> calculating for lfs compressed size threshold
> (git-p4.largeFileCompressedThreshold) comparisons.
>
> Take new approach using the NamedTemporaryFile()
> file-like object as input to the ZipFile() which
> auto-deletes after implicit close leaving with scope.
>
> Original code had double-open exception on Windows
> platform because file still open from NamedTemporaryFile()
> using generated filename instead of object.
>
> Thanks to Andrey for patiently suggesting several
> iterations on this change for avoiding exceptions!
>
> Also print error details after resulting IOError to make
> debugging cause of exception less mysterious when it has
> nothing to do with "git version recent enough."
>
> Signed-off-by: Philip.McGraw <Philip.McGraw@bentley.com>
> Reviewed-by: Andrey Mazo <ahippo@yandex.com>
> ---

Luke, does this look good?

I know Mazo is the only other contributor who has multiple commits
to git-p4.py in the past 2 years, to make Reviewed-by carry some
weight ;-) but as we have so small number of people touching this
script anyway, I'd rather see what the main contributor in the past
2 years thinks.

Thanks.

>  git-p4.py | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index c71a6832e2..33bdb14fd1 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1158,17 +1158,15 @@ def exceedsLargeFileThreshold(self, relPath, contents):
>          if gitConfigInt('git-p4.largeFileCompressedThreshold'):
>              contentsSize = sum(len(d) for d in contents)
>              if contentsSize <= gitConfigInt('git-p4.largeFileCompressedThreshold'):
>                  return False
>              contentTempFile = self.generateTempFile(contents)
> -            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
> -            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
> -            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
> -            zf.close()
> -            compressedContentsSize = zf.infolist()[0].compress_size
> +            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
> +            with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
> +                zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
> +                compressedContentsSize = zf.infolist()[0].compress_size
>              os.remove(contentTempFile)
> -            os.remove(compressedContentFile.name)
>              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
>                  return True
>          return False
>  
>      def addLargeFile(self, relPath):
> @@ -3512,12 +3510,13 @@ def importHeadRevision(self, revision):
>          details["time"] = res["time"]
>  
>          self.updateOptionDict(details)
>          try:
>              self.commit(details, self.extractFilesFromCommit(details), self.branch)
> -        except IOError:
> +        except IOError as err:
>              print("IO error with git fast-import. Is your git version recent enough?")
> +            print("IO error details: {}".format(err))
>              print(self.gitError.read())
>  
>      def openStreams(self):
>          self.importProcess = subprocess.Popen(["git", "fast-import"],
>                                                stdin=subprocess.PIPE,
>
> base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
> Pull-Request: https://github.com/gitgitgadget/git/pull/303

Re: [PATCH v2 1/1] git-p4: auto-delete named temporary file

[Luke Diamand]

On Tue, 27 Aug 2019 at 23:31, Junio C Hamano <gitster@pobox.com> wrote:
>
> Andrey Mazo <ahippo@yandex.ru> writes:
>
> > From: "Philip.McGraw" <Philip.McGraw@bentley.com>
> >
> > Avoid double-open exceptions on Windows platform when
> > calculating for lfs compressed size threshold
> > (git-p4.largeFileCompressedThreshold) comparisons.
> >
> > Take new approach using the NamedTemporaryFile()
> > file-like object as input to the ZipFile() which
> > auto-deletes after implicit close leaving with scope.
> >
> > Original code had double-open exception on Windows
> > platform because file still open from NamedTemporaryFile()
> > using generated filename instead of object.
> >
> > Thanks to Andrey for patiently suggesting several
> > iterations on this change for avoiding exceptions!
> >
> > Also print error details after resulting IOError to make
> > debugging cause of exception less mysterious when it has
> > nothing to do with "git version recent enough."
> >
> > Signed-off-by: Philip.McGraw <Philip.McGraw@bentley.com>
> > Reviewed-by: Andrey Mazo <ahippo@yandex.com>
> > ---
>
> Luke, does this look good?
>
> I know Mazo is the only other contributor who has multiple commits
> to git-p4.py in the past 2 years, to make Reviewed-by carry some
> weight ;-) but as we have so small number of people touching this
> script anyway, I'd rather see what the main contributor in the past
> 2 years thinks.

I think it looks reasonable.

Ack.


>
> Thanks.
>
> >  git-p4.py | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/git-p4.py b/git-p4.py
> > index c71a6832e2..33bdb14fd1 100755
> > --- a/git-p4.py
> > +++ b/git-p4.py
> > @@ -1158,17 +1158,15 @@ def exceedsLargeFileThreshold(self, relPath, contents):
> >          if gitConfigInt('git-p4.largeFileCompressedThreshold'):
> >              contentsSize = sum(len(d) for d in contents)
> >              if contentsSize <= gitConfigInt('git-p4.largeFileCompressedThreshold'):
> >                  return False
> >              contentTempFile = self.generateTempFile(contents)
> > -            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
> > -            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
> > -            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
> > -            zf.close()
> > -            compressedContentsSize = zf.infolist()[0].compress_size
> > +            compressedContentFile = tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
> > +            with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
> > +                zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
> > +                compressedContentsSize = zf.infolist()[0].compress_size
> >              os.remove(contentTempFile)
> > -            os.remove(compressedContentFile.name)
> >              if compressedContentsSize > gitConfigInt('git-p4.largeFileCompressedThreshold'):
> >                  return True
> >          return False
> >
> >      def addLargeFile(self, relPath):
> > @@ -3512,12 +3510,13 @@ def importHeadRevision(self, revision):
> >          details["time"] = res["time"]
> >
> >          self.updateOptionDict(details)
> >          try:
> >              self.commit(details, self.extractFilesFromCommit(details), self.branch)
> > -        except IOError:
> > +        except IOError as err:
> >              print("IO error with git fast-import. Is your git version recent enough?")
> > +            print("IO error details: {}".format(err))
> >              print(self.gitError.read())
> >
> >      def openStreams(self):
> >          self.importProcess = subprocess.Popen(["git", "fast-import"],
> >                                                stdin=subprocess.PIPE,
> >
> > base-commit: 1feeaaf26bff51996f9f96c6dc41ca0f95ab5fc4
> > Pull-Request: https://github.com/gitgitgadget/git/pull/303

Re: [PATCH v2 1/1] git-p4: auto-delete named temporary file

[Junio C Hamano]

> >> ...
> >>  Luke, does this look good?
> >>
> >>  I know Mazo is the only other contributor who has multiple commits
> >>  to git-p4.py in the past 2 years, to make Reviewed-by carry some
> >>  weight ;-) but as we have so small number of people touching this
> >>  script anyway, I'd rather see what the main contributor in the past
> >>  2 years thinks.
> >
> > I think it looks reasonable.
> >
> > Ack.
>
> Junio, does Philip need to resend a v3 with Luke's ack or will you just add it yourself while queuing the patch?
> (sorry, if you already picked up the patch -- I just didn't see it in the last What's cooking update)

Sorry, I do not recall the details of this old a thread but if I
recall correctly there was only a corrupt patch that I cannot use,
so somebody would need to send a patch that actually applies. With a
patch that applies, I can tweak the log message
with acked-by etc. just fine.

Thanks.

[PATCH 1/1] git-p4: auto-delete named temporary file

[Git Gadget]

From: "Philip.McGraw" <Philip.McGraw@bentley.com>

Take new approach using the NamedTemporaryFile()
file-like object as input to the ZipFile() which
auto-deletes after implicit close leaving with scope.

Original code produced double-open problems on Windows
platform from using already open NamedTemporaryFile()
generated filename instead of object.

Thanks to Andrey for patiently suggesting several
iterations on this change for avoiding exceptions!

Also print error details after resulting IOError to make
debugging cause of exception less mysterious when it has
nothing to do with "git version recent enough."

Signed-off-by: Philip.McGraw <Philip.McGraw@bentley.com>
---
 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index c71a6832e2..33bdb14fd1 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1160,13 +1160,11 @@ def exceedsLargeFileThreshold(self, relPath, contents):
             if contentsSize <=
gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return False
             contentTempFile = self.generateTempFile(contents)
-            compressedContentFile =
tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
-            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
-            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
-            zf.close()
-            compressedContentsSize = zf.infolist()[0].compress_size
+            compressedContentFile =
tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=True)
+            with zipfile.ZipFile(compressedContentFile, mode='w') as zf:
+                zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
+                compressedContentsSize = zf.infolist()[0].compress_size
             os.remove(contentTempFile)
-            os.remove(compressedContentFile.name)
             if compressedContentsSize >
gitConfigInt('git-p4.largeFileCompressedThreshold'):
                 return True
         return False
@@ -3514,8 +3512,9 @@ def importHeadRevision(self, revision):
         self.updateOptionDict(details)
         try:
             self.commit(details,
self.extractFilesFromCommit(details), self.branch)
-        except IOError:
+        except IOError as err:
             print("IO error with git fast-import. Is your git version
recent enough?")
+            print("IO error details: {}".format(err))
             print(self.gitError.read())

     def openStreams(self):

--
gitgitgadget

Re: [PATCH 1/1] git-p4: auto-delete named temporary file

[Junio C Hamano]

Funny that the patch is line-wrapped, which I do not recall ever
seeing in GGG-generated e-mails.  Dscho, do you know if anything
funny is going on?

Git Gadget <gitgitgadget@gmail.com> writes:

> From: "Philip.McGraw" <Philip.McGraw@bentley.com>
> ...
> diff --git a/git-p4.py b/git-p4.py
> index c71a6832e2..33bdb14fd1 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1160,13 +1160,11 @@ def exceedsLargeFileThreshold(self, relPath, contents):
>              if contentsSize <=
> gitConfigInt('git-p4.largeFileCompressedThreshold'):
>                  return False
>              contentTempFile = self.generateTempFile(contents)
> -            compressedContentFile =
> tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
> -            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
> -            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
> ...

Re: [PATCH 1/1] git-p4: auto-delete named temporary file

[Johannes Schindelin]

Hi Junio,

On Mon, 26 Aug 2019, Junio C Hamano wrote:

> Funny that the patch is line-wrapped, which I do not recall ever
> seeing in GGG-generated e-mails.  Dscho, do you know if anything
> funny is going on?

Yes, this was me trying to re-send the patch via GMail's web UI because
the first time GitGitGadget sent it, it did not get through (only the
cover letter did).

So I tried to fix the screw-up by sending manually, and screwed it up
even more.

Sorry about that.
Dscho

>
> Git Gadget <gitgitgadget@gmail.com> writes:
>
> > From: "Philip.McGraw" <Philip.McGraw@bentley.com>
> > ...
> > diff --git a/git-p4.py b/git-p4.py
> > index c71a6832e2..33bdb14fd1 100755
> > --- a/git-p4.py
> > +++ b/git-p4.py
> > @@ -1160,13 +1160,11 @@ def exceedsLargeFileThreshold(self, relPath, contents):
> >              if contentsSize <=
> > gitConfigInt('git-p4.largeFileCompressedThreshold'):
> >                  return False
> >              contentTempFile = self.generateTempFile(contents)
> > -            compressedContentFile =
> > tempfile.NamedTemporaryFile(prefix='git-p4-large-file', delete=False)
> > -            zf = zipfile.ZipFile(compressedContentFile.name, mode='w')
> > -            zf.write(contentTempFile, compress_type=zipfile.ZIP_DEFLATED)
> > ...
>

Re: [PATCH 1/1] git-p4: auto-delete named temporary file

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Yes, this was me trying to re-send the patch via GMail's web UI because
> the first time GitGitGadget sent it, it did not get through (only the
> cover letter did).

As long as that was manual screw-up, while fixing some glitches in
the machinery, that is fine.  I care about automation running
smoothly.

Thanks.

Re: [PATCH 1/1] git-p4: auto-delete named temporary file

[Johannes Schindelin]

Hi Junio,

On Wed, 28 Aug 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Yes, this was me trying to re-send the patch via GMail's web UI because
> > the first time GitGitGadget sent it, it did not get through (only the
> > cover letter did).
>
> As long as that was manual screw-up, while fixing some glitches in
> the machinery, that is fine.  I care about automation running
> smoothly.

Indeed, I also care about automation, as that keeps me sane (I would not
be able to do everything I do without offloading substantial parts to
Azure Pipelines, like GitGitGadget).

I am quite a bit worried about this, as it seemed to happen at least
three times. I'll see if it happens again, and when it does, I will try
to save the mbox files as build artifacts, and then add another job that
I can run manually to re-send the mbox files that did not make it to the
mailing list (identified by the absence from public-inbox).

At the moment, I am still rather busy elsewhere, trying to catch up
after three weeks that I spent (or tried to spend) mostly offline.

And I am really sorry for not writing a reply to the thread when I tried
to re-send the patch manually, describing what I was doing, and why. I
will try to do better next time!

Ciao,
Dscho