Sergey Yurzin <jurzin.s@xxxxxxxxx> 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 <sergei_iurzin@xxxxxxxx>
>
> ---
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