git-p4: do not fail in verbose mode for missing fileSize

[CaballerosTeam]

git-p4 might fall in verbose mode with KeyError "fileSize" exception. Suggested don't print file size if it not known.

Signed-off-by: Sergey Yurzin jurzin.s@gmail.com

[submitgit]

@CaballerosTeam sent this commit (8d1b103...da72693) as a patch to the mailing list with submitGit - here on public-inbox, MARC

[dscho]

This patch is stuck waiting for feedback from the patch author: https://public-inbox.org/git/xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com/

[dscho]

@CaballerosTeam care to respond?

[dscho]

@CaballerosTeam please review the answer provided in https://public-inbox.org/git/xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com/, and answer as indicated on the bottom of that page (or if you prefer, say, GMail's UI, you can try to follow this advice).

[dscho]

@CaballerosTeam gentle ping?

[dscho]

@CaballerosTeam another gentle ping?

If you are no longer interested in this patch, please close this PR.

[dscho]

@CaballerosTeam please do review the answer provided in https://public-inbox.org/git/xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com/, and answer.

[dscho]

@CaballerosTeam ping?

[dscho]

@CaballerosTeam quoting that mail for your convenience:

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

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.

So maybe now you can address that feedback, by replying to that mail either by following the instructions on the bottom of the linked page or by following the advice given in https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis?

[CaballerosTeam]

Hey everyone, actually I can't recall exactly why that patch is got stuck in middle of 2017. It seems to me that there was a discussion with one of the reviewers, which remained without final solution.

So could someone give me a hint what your expectations are? Provide a more sensible commit message to a reader? or dig deeper in the root case and figure out why fileSize is not filled out properly rather than mitigate the issue?

[dscho]

I can't recall exactly why that patch is got stuck in middle of 2017.

I can tell you: because you had not responded to Junio's mail. In other words, there was no discussion, as a discussion requires not only one side to speak, but two.

That's why I keep pinging you, and even served the fries on a silver platter, so to say, by pasting the mail into a comment in this PR so that you would not need to follow the link to the mail that I gave to you earlier.

So could someone give me a hint what your expectations are? Provide a more sensible commit message to a reader?

Yes, of course. There was even a copy-pasteable suggestion how to improve the first line of the commit message:

Subject: [PATCH] git-p4: do not fail in verbose mode for missing fileSize

Also, Junio pointed out to you that you need to add your sign-off, mentioning the documentation about contributing patches: https://git-scm.com/docs/SubmittingPatches

dig deeper in the root case and figure out why fileSize is not filled out properly rather than mitigate the issue?

Well, the rather clear indication that an explanation for this change is missing from the commit message ("The blank space above is to explain why this change is needed and a good idea") should be enough to tell you that yes, you should know why your patch solves the problem, and explain that in the commit message so that others will see why it is a patch that we would want to take.

One thing that puzzles me to no end is why you left this statement unanswered:

it is not at all obvious if this is fixing the problem or
sweeping the problem under the rug.

You could answer this either by writing a convincing commit message, in the indicated way (Junio did not spend that much time writing the very detailed review expecting that you would totally ignore it).

Or, if the suspicion that the patch might be "sweeping the problem under the rug" is actually correct, you could change the patch to address the underlying problem instead, then submit a new iteration of the patch (just force-push and use submitGit again).

In both cases, however, it will require you to "dig deeper" to actually understand what is going on, and to know whether your patch is fixing the problem or just papering over a not yet understood deeper problem.

If all of this seems like too much work, just close the Pull Request and forget about it.

[dscho]

it is not at all obvious if this is fixing the problem or
sweeping the problem under the rug.

You could answer this either by writing a convincing commit message, in the indicated way (Junio did not spend that much time writing the very detailed review expecting that you would totally ignore it).

One very obvious way to answer this concern of Junio's could have been to point out that both 4d25dc4 and d2176a5 added conditional code contingent on self.stream_file having the fileSize attribute, so yes, there must have been a real reason for this check, and yeah, you obviously encountered such a scenario, why would you have written this patch otherwise.

And a very obvious way to write a commit message would use these two commits as exhibits A and B when "building the case" for this patch. Something like this:

Just like 4d25dc4 (git-p4: check free space during streaming, 2015-09-26) and d2176a5 (git-p4: add file streaming progress in verbose mode, 2015-09-26), streamOneP4File() needs to account for the possibility that self.stream_file misses the fileSize attribute.

And then you would also Cc: the author of these two patches, @larsxschneider, so that they can possibly contribute the underlying reasons if they remember.

[dscho]

Oh, and the commit message should of course also mention that with this patch, there is code path left that accesses the fileSize attribute without checking for its existence.

[CaballerosTeam]

Thank you very much for such detailed reply. I found an email chain with Lars Schneider (@larsxschneider) there we discussed that patch and borrowed from it couple of ideas regarding to circumstances. Please, let me know if the following message makes sense:

streamOneP4File method needs to check self.stream_file for existence of fileSize key, otherwise it fails with KeyError exception in case of verbose output mode. It looks like P4 server not always post fileSize key in Python marshaled object. It was uncovered during migration from older server version: 2009 server version branch.

Signed-off-by: Sergey Yurzin jurzin.s@gmail.com

with best regards,
Sergey

[dscho]

As I already pointed out, I would have preferred something like this:

Just like 4d25dc4 (git-p4: check free space during streaming, 2015-09-26) and d2176a5 (git-p4: add file streaming progress in verbose mode, 2015-09-26), streamOneP4File() needs to account for the possibility that self.stream_file misses the fileSize attribute.

But you do what you do, and I'm done here.

[CaballerosTeam]

I'm going to close this pull request since the issue was fixed in 0742b7c