Re: [PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'

[Thandesha VK]

Ah. I didn't realize the script is not using p4 sizes to get the size.
I assumed that it is using p4 sizes. Now I am looking at it using p4
-G print.
However, when the stack trace happened, I verified what is wrong and
found out that the fileSize key is not returned for "p4 -G sizes"
command.


So what I am saying is that we cannot use p4 sizes in this case to
solve the problem as even p4 sizes will fail with the same fileSize
not found stack trace.
We can continue as this is not as Critical. However it is a good idea
to let user know that they need to take action to correct this file at
the perforce side.

So irrespective of we are using p4 print or p4 sizes, the fileSize is
not returned in some cases and warning or aborting is something we
need to do.
Just ignoring may not be a good choice.

On Tue, Apr 17, 2018 at 2:38 PM, Mazo, Andrey  wrote:
> Thandesha,
>
> If I read your patch correctly, it adds a warning in case `p4 -G print` 
> doesn't return "fileSize" (not `p4 sizes`).
> I don't see `p4 sizes` being used by git-p4 at all.
> As I said earlier, for our ancient Perforce server, `p4 -G print` _never_ 
> returns "fileSize".
> So, it's definitely not a reason to abort.
>
> Thank you,
> Andrey
>
> From: Thandesha VK 
>> My fix is for the case where p4 -G sizes not returning the key and
>> value for fileSize. This can happen in some cases. Only option at that
>> point of time is to warn the user about the problematic file and keep
>> moving (or should we abort??)
>>
>> Thanks
>> Thandesha
>>
>> On Tue, Apr 17, 2018 at 2:18 PM, Mazo, Andrey  wrote:
>>> Luke,
>>>
>>> Thank you for reviewing and acking my patch!
>>> By the way, did you see Thandesha's proposed patch [1] to print a warning 
>>> in case of the missing "fileSize" attribute?
>>> Should we go that route instead?
>>> Or should we try harder to get the size by running `p4 -G sizes`?
>>>
>>> [1]  
>>> https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#m6053d2031020e08edd24ada6c9eb49721ebc4e27
>>>
>>> Thank you,
>>> Andrey
>>>
>>> From: Luke Diamand 
 On Tue, 17 Apr 2018, 09:22 Andrey Mazo,  wrote:
>  Perforce server 2007.2 (and maybe others) doesn't return "fileSize"
> attribute in its reply to `p4 -G print` command.
> This causes the following traceback when running `git p4 sync --verbose`:
> """
> Traceback (most recent call last):
>   File "/usr/libexec/git-core/git-p4", line 3839, in 
> main()
>   File "/usr/libexec/git-core/git-p4", line 3833, in main
> if not cmd.run(args):
>   File "/usr/libexec/git-core/git-p4", line 3567, in run
> self.importChanges(changes)
>   File "/usr/libexec/git-core/git-p4", line 3233, in importChanges
> self.commit(description, filesForCommit, branch, parent)
>   File "/usr/libexec/git-core/git-p4", line 2855, in commit
> self.streamP4Files(files)
>   File "/usr/libexec/git-core/git-p4", line 2747, in streamP4Files
> cb=streamP4FilesCbSelf)
>   File "/usr/libexec/git-core/git-p4", line 552, in p4CmdList
> cb(entry)
>   File "/usr/libexec/git-core/git-p4", line 2741, in 
> streamP4FilesCbSelf
> self.streamP4FilesCb(entry)
>   File "/usr/libexec/git-core/git-p4", line 2689, in streamP4FilesCb
> self.streamOneP4File(self.stream_file, self.stream_contents)
>   File "/usr/libexec/git-core/git-p4", line 2566, in streamOneP4File
> size = int(self.stream_file['fileSize'])
> KeyError: 'fileSize'
> """
>
> Fix this by omitting the file size information from the verbose print out.
> Also, don't use "self.stream_file" directly,
> but rather use passed in "file" argument.
> (which point to the same "self.stream_file" for all existing callers)
>
> Signed-off-by: Andrey Mazo 
> ---
>  git -p4.py | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc6..6f05f915a 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2566,8 +2566,12 @@ class P4Sync(Command, P4UserMap):
>  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))
> +size = file.get('fileSize', None)
> +if size is None:
> +sizeStr = ''
> +else:
> +sizeStr = ' (%i MB)' % (int(size)/1024/1024)
> +sys.stdout.write('\r%s --> %s%s\n' % (file['depotFile'], 

Re: [PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'

[Mazo, Andrey]

Thandesha,

If I read your patch correctly, it adds a warning in case `p4 -G print` doesn't 
return "fileSize" (not `p4 sizes`).
I don't see `p4 sizes` being used by git-p4 at all.
As I said earlier, for our ancient Perforce server, `p4 -G print` _never_ 
returns "fileSize".
So, it's definitely not a reason to abort.

Thank you,
Andrey

From: Thandesha VK 
> My fix is for the case where p4 -G sizes not returning the key and
> value for fileSize. This can happen in some cases. Only option at that
> point of time is to warn the user about the problematic file and keep
> moving (or should we abort??)
> 
> Thanks
> Thandesha
> 
> On Tue, Apr 17, 2018 at 2:18 PM, Mazo, Andrey  wrote:
>> Luke,
>>
>> Thank you for reviewing and acking my patch!
>> By the way, did you see Thandesha's proposed patch [1] to print a warning in 
>> case of the missing "fileSize" attribute?
>> Should we go that route instead?
>> Or should we try harder to get the size by running `p4 -G sizes`?
>>
>> [1]  
>> https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#m6053d2031020e08edd24ada6c9eb49721ebc4e27
>>
>> Thank you,
>> Andrey
>>
>> From: Luke Diamand 
>>> On Tue, 17 Apr 2018, 09:22 Andrey Mazo,  wrote:
  Perforce server 2007.2 (and maybe others) doesn't return "fileSize"
 attribute in its reply to `p4 -G print` command.
 This causes the following traceback when running `git p4 sync --verbose`:
 """
 Traceback (most recent call last):
   File "/usr/libexec/git-core/git-p4", line 3839, in 
 main()
   File "/usr/libexec/git-core/git-p4", line 3833, in main
 if not cmd.run(args):
   File "/usr/libexec/git-core/git-p4", line 3567, in run
 self.importChanges(changes)
   File "/usr/libexec/git-core/git-p4", line 3233, in importChanges
 self.commit(description, filesForCommit, branch, parent)
   File "/usr/libexec/git-core/git-p4", line 2855, in commit
 self.streamP4Files(files)
   File "/usr/libexec/git-core/git-p4", line 2747, in streamP4Files
 cb=streamP4FilesCbSelf)
   File "/usr/libexec/git-core/git-p4", line 552, in p4CmdList
 cb(entry)
   File "/usr/libexec/git-core/git-p4", line 2741, in 
streamP4FilesCbSelf
 self.streamP4FilesCb(entry)
   File "/usr/libexec/git-core/git-p4", line 2689, in streamP4FilesCb
 self.streamOneP4File(self.stream_file, self.stream_contents)
   File "/usr/libexec/git-core/git-p4", line 2566, in streamOneP4File
 size = int(self.stream_file['fileSize'])
 KeyError: 'fileSize'
 """

 Fix this by omitting the file size information from the verbose print out.
 Also, don't use "self.stream_file" directly,
 but rather use passed in "file" argument.
 (which point to the same "self.stream_file" for all existing callers)

 Signed-off-by: Andrey Mazo 
 ---
  git -p4.py | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

 diff --git a/git-p4.py b/git-p4.py
 index 7bb9cadc6..6f05f915a 100755
 --- a/git-p4.py
 +++ b/git-p4.py
 @@ -2566,8 +2566,12 @@ class P4Sync(Command, P4UserMap):
  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))
 +    size = file.get('fileSize', None)
 +    if size is None:
 +    sizeStr = ''
 +    else:
 +    sizeStr = ' (%i MB)' % (int(size)/1024/1024)
 +    sys.stdout.write('\r%s --> %s%s\n' % (file['depotFile'], 
 relPath, sizeStr))
  sys.stdout.flush()

  (type_base, type_mods) = split_p4_type(file["type"])
 --
 2.16.1

>>> Thanks, that looks like a good fix to me.  Ack.
>
> -- 
> Thanks & Regards
> Thandesha VK | Cellphone +1 (703) 459-5386

Re: [PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'

[Thandesha VK]

My fix is for the case where p4 -G sizes not returning the key and
value for fileSize. This can happen in some cases. Only option at that
point of time is to warn the user about the problematic file and keep
moving (or should we abort??)

Thanks
Thandesha

On Tue, Apr 17, 2018 at 2:18 PM, Mazo, Andrey  wrote:
> Luke,
>
> Thank you for reviewing and acking my patch!
> By the way, did you see Thandesha's proposed patch [1] to print a warning in 
> case of the missing "fileSize" attribute?
> Should we go that route instead?
> Or should we try harder to get the size by running `p4 -G sizes`?
>
> [1] 
> https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#m6053d2031020e08edd24ada6c9eb49721ebc4e27
>
> Thank you,
> Andrey
>
> From: Luke Diamand 
>> On Tue, 17 Apr 2018, 09:22 Andrey Mazo,  wrote:
>>>  Perforce server 2007.2 (and maybe others) doesn't return "fileSize"
>>> attribute in its reply to `p4 -G print` command.
>>> This causes the following traceback when running `git p4 sync --verbose`:
>>> """
>>> Traceback (most recent call last):
>>>   File "/usr/libexec/git-core/git-p4", line 3839, in 
>>> main()
>>>   File "/usr/libexec/git-core/git-p4", line 3833, in main
>>> if not cmd.run(args):
>>>   File "/usr/libexec/git-core/git-p4", line 3567, in run
>>> self.importChanges(changes)
>>>   File "/usr/libexec/git-core/git-p4", line 3233, in importChanges
>>> self.commit(description, filesForCommit, branch, parent)
>>>   File "/usr/libexec/git-core/git-p4", line 2855, in commit
>>> self.streamP4Files(files)
>>>   File "/usr/libexec/git-core/git-p4", line 2747, in streamP4Files
>>> cb=streamP4FilesCbSelf)
>>>   File "/usr/libexec/git-core/git-p4", line 552, in p4CmdList
>>> cb(entry)
>>>   File "/usr/libexec/git-core/git-p4", line 2741, in streamP4FilesCbSelf
>>> self.streamP4FilesCb(entry)
>>>   File "/usr/libexec/git-core/git-p4", line 2689, in streamP4FilesCb
>>> self.streamOneP4File(self.stream_file, self.stream_contents)
>>>   File "/usr/libexec/git-core/git-p4", line 2566, in streamOneP4File
>>> size = int(self.stream_file['fileSize'])
>>> KeyError: 'fileSize'
>>> """
>>>
>>> Fix this by omitting the file size information from the verbose print out.
>>> Also, don't use "self.stream_file" directly,
>>> but rather use passed in "file" argument.
>>> (which point to the same "self.stream_file" for all existing callers)
>>>
>>> Signed-off-by: Andrey Mazo 
>>> ---
>>>  git -p4.py | 8 ++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/git-p4.py b/git-p4.py
>>> index 7bb9cadc6..6f05f915a 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -2566,8 +2566,12 @@ class P4Sync(Command, P4UserMap):
>>>  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))
>>> +size = file.get('fileSize', None)
>>> +if size is None:
>>> +sizeStr = ''
>>> +else:
>>> +sizeStr = ' (%i MB)' % (int(size)/1024/1024)
>>> +sys.stdout.write('\r%s --> %s%s\n' % (file['depotFile'], 
>>> relPath, sizeStr))
>>>  sys.stdout.flush()
>>>
>>>  (type_base, type_mods) = split_p4_type(file["type"])
>>> --
>>> 2.16.1
>>>
>> Thanks, that looks like a good fix to me.  Ack.



-- 
Thanks & Regards
Thandesha VK | Cellphone +1 (703) 459-5386

Re: [PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'

[Mazo, Andrey]

Luke,

Thank you for reviewing and acking my patch!
By the way, did you see Thandesha's proposed patch [1] to print a warning in 
case of the missing "fileSize" attribute?
Should we go that route instead?
Or should we try harder to get the size by running `p4 -G sizes`?

[1] 
https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#m6053d2031020e08edd24ada6c9eb49721ebc4e27

Thank you,
Andrey

From: Luke Diamand 
> On Tue, 17 Apr 2018, 09:22 Andrey Mazo,  wrote:
>>  Perforce server 2007.2 (and maybe others) doesn't return "fileSize"
>> attribute in its reply to `p4 -G print` command.
>> This causes the following traceback when running `git p4 sync --verbose`:
>> """
>>     Traceback (most recent call last):
>>       File "/usr/libexec/git-core/git-p4", line 3839, in 
>>         main()
>>       File "/usr/libexec/git-core/git-p4", line 3833, in main
>>         if not cmd.run(args):
>>       File "/usr/libexec/git-core/git-p4", line 3567, in run
>>         self.importChanges(changes)
>>       File "/usr/libexec/git-core/git-p4", line 3233, in importChanges
>>         self.commit(description, filesForCommit, branch, parent)
>>       File "/usr/libexec/git-core/git-p4", line 2855, in commit
>>         self.streamP4Files(files)
>>       File "/usr/libexec/git-core/git-p4", line 2747, in streamP4Files
>>         cb=streamP4FilesCbSelf)
>>       File "/usr/libexec/git-core/git-p4", line 552, in p4CmdList
>>         cb(entry)
>>       File "/usr/libexec/git-core/git-p4", line 2741, in streamP4FilesCbSelf
>>         self.streamP4FilesCb(entry)
>>       File "/usr/libexec/git-core/git-p4", line 2689, in streamP4FilesCb
>>         self.streamOneP4File(self.stream_file, self.stream_contents)
>>       File "/usr/libexec/git-core/git-p4", line 2566, in streamOneP4File
>>         size = int(self.stream_file['fileSize'])
>>     KeyError: 'fileSize'
>> """
>> 
>> Fix this by omitting the file size information from the verbose print out.
>> Also, don't use "self.stream_file" directly,
>> but rather use passed in "file" argument.
>> (which point to the same "self.stream_file" for all existing callers)
>> 
>> Signed-off-by: Andrey Mazo 
>> ---
>>  git -p4.py | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 7bb9cadc6..6f05f915a 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2566,8 +2566,12 @@ class P4Sync(Command, P4UserMap):
>>          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))
>> +            size = file.get('fileSize', None)
>> +            if size is None:
>> +                sizeStr = ''
>> +            else:
>> +                sizeStr = ' (%i MB)' % (int(size)/1024/1024)
>> +            sys.stdout.write('\r%s --> %s%s\n' % (file['depotFile'], 
>> relPath, sizeStr))
>>              sys.stdout.flush()
>> 
>>          (type_base, type_mods) = split_p4_type(file["type"])
>> -- 
>> 2.16.1
>>
> Thanks, that looks like a good fix to me.  Ack.

[PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'

[Andrey Mazo]

Perforce server 2007.2 (and maybe others) doesn't return "fileSize"
attribute in its reply to `p4 -G print` command.
This causes the following traceback when running `git p4 sync --verbose`:
"""
Traceback (most recent call last):
  File "/usr/libexec/git-core/git-p4", line 3839, in 
main()
  File "/usr/libexec/git-core/git-p4", line 3833, in main
if not cmd.run(args):
  File "/usr/libexec/git-core/git-p4", line 3567, in run
self.importChanges(changes)
  File "/usr/libexec/git-core/git-p4", line 3233, in importChanges
self.commit(description, filesForCommit, branch, parent)
  File "/usr/libexec/git-core/git-p4", line 2855, in commit
self.streamP4Files(files)
  File "/usr/libexec/git-core/git-p4", line 2747, in streamP4Files
cb=streamP4FilesCbSelf)
  File "/usr/libexec/git-core/git-p4", line 552, in p4CmdList
cb(entry)
  File "/usr/libexec/git-core/git-p4", line 2741, in streamP4FilesCbSelf
self.streamP4FilesCb(entry)
  File "/usr/libexec/git-core/git-p4", line 2689, in streamP4FilesCb
self.streamOneP4File(self.stream_file, self.stream_contents)
  File "/usr/libexec/git-core/git-p4", line 2566, in streamOneP4File
size = int(self.stream_file['fileSize'])
KeyError: 'fileSize'
"""

Fix this by omitting the file size information from the verbose print out.
Also, don't use "self.stream_file" directly,
but rather use passed in "file" argument.
(which point to the same "self.stream_file" for all existing callers)

Signed-off-by: Andrey Mazo 
---
 git-p4.py | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc6..6f05f915a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2566,8 +2566,12 @@ class P4Sync(Command, P4UserMap):
 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))
+size = file.get('fileSize', None)
+if size is None:
+sizeStr = ''
+else:
+sizeStr = ' (%i MB)' % (int(size)/1024/1024)
+sys.stdout.write('\r%s --> %s%s\n' % (file['depotFile'], relPath, 
sizeStr))
 sys.stdout.flush()
 
 (type_base, type_mods) = split_p4_type(file["type"])
-- 
2.16.1