Re: [PATCH] Fix KeyError "fileSize" in verbose mode
- From
Lars Schneider <larsxschneider@gmail.com>
- Date
- Jun 13, 2017, 07:35 UTC
- Message-ID
- <B3689D6E-A6B8-42F8-9105-242178FE16E0@gmail.com>
- In-Reply-To
- <xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com>
Show 28 quoted lines
> On 12 Jun 2017, at 19:11, Junio C Hamano <gitster@pobox.com> wrote:
>
> Sergey Yurzin <jurzin.s@gmail.com> 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
Show 34 quoted lines
> 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