Re: [PATCH] Fix KeyError "fileSize" in verbose mode

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2017, 17:11 UTC
Message-ID: <xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102015c9b535a84-fd59d55d-387a-419e-b8ec-439873c4b7f5-000000@eu-west-1.amazonses.com>

Sergey Yurzin <jurzin.s@gmail.com> 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@epam.com>
>
> ---

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).

Show 19 quoted lines

>  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