# [PATCH] Fix KeyError "fileSize" in verbose mode 3 messages from 2017-06-12 to 2017-06-13. Participants: Sergey Yurzin, Junio C Hamano, Lars Schneider. Thread: https://gitlist.dev/t/46157 ## Sergey Yurzin, 2017-06-12 08:02 Subject: [PATCH] Fix KeyError "fileSize" in verbose mode Message-ID: <0102015c9b535a84-fd59d55d-387a-419e-b8ec-439873c4b7f5-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102015c9b535a84-fd59d55d-387a-419e-b8ec-439873c4b7f5-000000%40eu-west-1.amazonses.com ``` From: Sergei Iurzin --- git-p4.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/git-p4.py b/git-p4.py index 8d151da91b969..b3666eddf12e3 100755 --- a/git-p4.py +++ b/git-p4.py @@ -2523,8 +2523,11 @@ def streamOneP4File(self, file, contents): relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes) relPath = self.encodeWithUTF8(relPath) if verbose: - size = int(self.stream_file['fileSize']) - sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024)) + if 'fileSize' in self.stream_file: + size = int(self.stream_file['fileSize']) + sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024)) + else: + sys.stdout.write('\r%s --> %s\n' % (file['depotFile'], relPath)) sys.stdout.flush() (type_base, type_mods) = split_p4_type(file["type"]) -- https://github.com/git/git/pull/373 ``` ## Junio C Hamano, 2017-06-12 17:11 Subject: Re: [PATCH] Fix KeyError "fileSize" in verbose mode Message-ID: URL: https://gitlist.dev/e/xmqq1sqpp1vv.fsf%40gitster.mtv.corp.google.com In-Reply-To: <0102015c9b535a84-fd59d55d-387a-419e-b8ec-439873c4b7f5-000000@eu-west-1.amazonses.com> ``` Sergey Yurzin writes: > Subject: Re: [PATCH] Fix KeyError "fileSize" in verbose mode The convention around here is to think how a single change appears in "git shortlog --no-merges" output. The above commit title does not tell readers of "shortlog" what the change is about in the context of the whole system. Subject: [PATCH] git-p4: do not fail in verbose mode for missing `fileSize` or something? > From: Sergei Iurzin > > --- The blank space above is to explain why this change is needed and a good idea, followed by your Signed-off-by: line (please see Documentation/SubmittingPatches). > git-p4.py | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/git-p4.py b/git-p4.py > index 8d151da91b969..b3666eddf12e3 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -2523,8 +2523,11 @@ def streamOneP4File(self, file, contents): > relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes) > relPath = self.encodeWithUTF8(relPath) > if verbose: > - size = int(self.stream_file['fileSize']) > - sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024)) > + if 'fileSize' in self.stream_file: > + size = int(self.stream_file['fileSize']) > + sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024)) > + else: > + sys.stdout.write('\r%s --> %s\n' % (file['depotFile'], relPath)) > sys.stdout.flush() I can see from your patch that self.stream_file[] sometimes may not have `fileSize` and when that happens the current code will barf. I also can see that with your patch, the code will NOT barf but output would lack the size information (obviously, because it is not available). However, it is not at all obvious if this is fixing the problem or sweeping the problem under the rug. The proposed log message you write before the patch is the ideal place to say something like In such and such circumstances, it is perfectly normal that P4Sync.stream_file does not know its file and lacks `fileSize`. streamOneP4File() method, however, assumes that this key is always available and tries to write it under the verbose mode. Check the existence of the `fileSize` key and show it only when available. Note that the above _assumes_ that a stream_file that lacks `fileSize` is perfectly normal; if that assumption is incorrect, then perhaps a real fix may be to set `fileSize` in the codepath that forgets to set it (I am not a git-p4 expert, and I am asking Luke to review this patch by CC'ing him). Also note that in a real log message that is helpful for future readers, "In such and such circumstances" in the above illustration needs to become a more concrete description. Thanks, and welcome to Git development community. > (type_base, type_mods) = split_p4_type(file["type"]) > > -- > https://github.com/git/git/pull/373 ``` ## Lars Schneider, 2017-06-13 07:35 Subject: Re: [PATCH] Fix KeyError "fileSize" in verbose mode Message-ID: URL: https://gitlist.dev/e/B3689D6E-A6B8-42F8-9105-242178FE16E0%40gmail.com In-Reply-To: ``` > On 12 Jun 2017, at 19:11, Junio C Hamano wrote: > > Sergey Yurzin writes: > >> Subject: Re: [PATCH] Fix KeyError "fileSize" in verbose mode > > ... > >> git-p4.py | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/git-p4.py b/git-p4.py >> index 8d151da91b969..b3666eddf12e3 100755 >> --- a/git-p4.py >> +++ b/git-p4.py >> @@ -2523,8 +2523,11 @@ def streamOneP4File(self, file, contents): >> relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes) >> relPath = self.encodeWithUTF8(relPath) >> if verbose: >> - size = int(self.stream_file['fileSize']) >> - sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024)) >> + if 'fileSize' in self.stream_file: >> + size = int(self.stream_file['fileSize']) >> + sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024)) >> + else: >> + sys.stdout.write('\r%s --> %s\n' % (file['depotFile'], relPath)) >> sys.stdout.flush() > Are you working with P4 Streams? I think I ran into the same problem if Streams are involved and I solved it like this: if verbose: - size = int(self.stream_file['fileSize']) + if 'fileSize' in self.stream_file: + size = int(self.stream_file['fileSize']) + elif 'streamContentSize' in self.stream_file: + size = int(self.stream_file['streamContentSize']) + else: + size = 0 sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024)) sys.stdout.flush() However "streamContentSize" is something we define ourselves elsewhere in git-p4 and not something we get from P4. Therefore it might be garbage. I haven't looked into it further, yet. Your 'fileSize' check is definitively correct! Cheers, Lars > I can see from your patch that self.stream_file[] sometimes may not > have `fileSize` and when that happens the current code will barf. I > also can see that with your patch, the code will NOT barf but output > would lack the size information (obviously, because it is not > available). > > However, it is not at all obvious if this is fixing the problem or > sweeping the problem under the rug. The proposed log message you > write before the patch is the ideal place to say something like > > In such and such circumstances, it is perfectly normal that > P4Sync.stream_file does not know its file and lacks `fileSize`. > streamOneP4File() method, however, assumes that this key is > always available and tries to write it under the verbose mode. > > Check the existence of the `fileSize` key and show it only when > available. > > Note that the above _assumes_ that a stream_file that lacks > `fileSize` is perfectly normal; if that assumption is incorrect, > then perhaps a real fix may be to set `fileSize` in the codepath > that forgets to set it (I am not a git-p4 expert, and I am asking > Luke to review this patch by CC'ing him). > > Also note that in a real log message that is helpful for future > readers, "In such and such circumstances" in the above illustration > needs to become a more concrete description. > > Thanks, and welcome to Git development community. > >> (type_base, type_mods) = split_p4_type(file["type"]) >> >> -- >> https://github.com/git/git/pull/373 ```