Message ID | 7e59b5cec2f267820feeeeb63a20814fe67d61e3.1566876175.git.ahippo@yandex.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | git-p4: auto-delete named temporary file | expand |
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
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
> >> ... > >> 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.
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,