git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
@ 2018-04-16 23:35 Thandesha VK
  2018-04-17 16:00 ` Mazo, Andrey
  2018-04-17 16:22 ` Andrey Mazo
  0 siblings, 2 replies; 10+ messages in thread
From: Thandesha VK @ 2018-04-16 23:35 UTC (permalink / raw)
  To: git

git p4 clone fails when p4 sizes does not return 'fileSize' key. There
are few cases when p4 sizes returens 0 size and with marshaled output,
it doesn’t return the fileSize attribute.

Here is the demonstration and potential fix



$ cd /tmp/git/



$ git remote -v

origin  https://github.com/git/git.git (fetch)

origin  https://github.com/git/git.git (push)



$ git branch  -v

* master fe0a9eaf3 Merge branch 'svn/authors-prog-2' of
git://bogomips.org/git-svn



Problem:



$ /tmp/git/git-p4.py clone //depot/<path>/@all   . –verbose

.

.

.

Traceback (most recent call last):

  File "/tmp/git/git-p4.py", line 3840, in <module>

    main()

  File "/tmp/git/git-p4.py", line 3834, in main

    if not cmd.run(args):

  File "/tmp/git/git-p4.py", line 3706, in run

    if not P4Sync.run(self, depotPaths):

  File "/tmp/git/git-p4.py", line 3568, in run

    self.importChanges(changes)

  File "/tmp/git/git-p4.py", line 3240, in importChanges

    self.initialParent)

  File "/tmp/git/git-p4.py", line 2858, in commit

    self.streamP4Files(files)

  File "/tmp/git/git-p4.py", line 2750, in streamP4Files

    cb=streamP4FilesCbSelf)

  File "/tmp/git/git-p4.py", line 552, in p4CmdList

    cb(entry)

  File "/tmp/git/git-p4.py", line 2744, in streamP4FilesCbSelf

    self.streamP4FilesCb(entry)

  File "/tmp/git/git-p4.py", line 2692, in streamP4FilesCb

    self.streamOneP4File(self.stream_file, self.stream_contents)

  File "/tmp/git/git-p4.py", line 2569, in streamOneP4File

    size = int(self.stream_file['fileSize'])

KeyError: 'fileSize'



Signature of the sizes output resulting in this problem:

$ p4 -p <port>  sizes //foo.c

//foo.c#5 <n/a> bytes



$ p4 -p <port>  -G sizes //foo.c

{scodesstats    depotFiles4//fooc.c50



Signature for a file without problem:



$ p4 -p <port>  sizes //bar.c

//bar.c#5 1105 bytes



$ p4 -p <port> -G  sizes //bar.c

{scodesstats    depotFiles;//bar.csrevs5fileSizes11050



Patch:

$ git diff

diff --git a/git-p4.py b/git-p4.py

index 7bb9cadc6..f908e805e 100755

--- a/git-p4.py

+++ b/git-p4.py

@@ -2565,7 +2565,7 @@ class P4Sync(Command, P4UserMap):

     def streamOneP4File(self, file, contents):

         relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)

         relPath = self.encodeWithUTF8(relPath)

-        if verbose:

+        if verbose and '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))

             sys.stdout.flush()



Thanks & Regards

Thandesha

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-16 23:35 [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key Thandesha VK
@ 2018-04-17 16:00 ` Mazo, Andrey
  2018-04-17 16:22 ` Andrey Mazo
  1 sibling, 0 replies; 10+ messages in thread
From: Mazo, Andrey @ 2018-04-17 16:00 UTC (permalink / raw)
  To: Thandesha VK, git

Huh, I actually have a slightly different fix for the same issue.
It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
I'll (try to) post it as a reply to this email.

Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.

Thank you,
Andrey

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-16 23:35 [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key Thandesha VK
  2018-04-17 16:00 ` Mazo, Andrey
@ 2018-04-17 16:22 ` Andrey Mazo
  2018-04-17 17:21   ` Thandesha VK
  1 sibling, 1 reply; 10+ messages in thread
From: Andrey Mazo @ 2018-04-17 16:22 UTC (permalink / raw)
  To: git; +Cc: Andrey Mazo, luke, gvanburgh, larsxschneider, miguel.torroja, thanvk

Huh, I actually have a slightly different fix for the same issue.
It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.

Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.

Andrey Mazo (1):
  git-p4: fix `sync --verbose` traceback due to 'fileSize'

 git-p4.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


base-commit: 468165c1d8a442994a825f3684528361727cd8c0
-- 
2.16.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-17 16:22 ` Andrey Mazo
@ 2018-04-17 17:21   ` Thandesha VK
  2018-04-17 17:33     ` Mazo, Andrey
  0 siblings, 1 reply; 10+ messages in thread
From: Thandesha VK @ 2018-04-17 17:21 UTC (permalink / raw)
  To: Andrey Mazo; +Cc: git, luke, gvanburgh, larsxschneider, miguel.torroja

*I* think keeping the filesize info is better with --verbose option as
that gives some clue about the file we are working on. What do you
think?
Script has similar checks of key existence at other places where it is
looking for fileSize.

On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo <amazo@checkvideo.com> wrote:
> Huh, I actually have a slightly different fix for the same issue.
> It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
>
> Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.
>
> Andrey Mazo (1):
>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>
>  git-p4.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
>
> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
> --
> 2.16.1
>



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-17 17:21   ` Thandesha VK
@ 2018-04-17 17:33     ` Mazo, Andrey
  2018-04-17 18:01       ` Thandesha VK
  0 siblings, 1 reply; 10+ messages in thread
From: Mazo, Andrey @ 2018-04-17 17:33 UTC (permalink / raw)
  To: Thandesha VK; +Cc: git, luke, gvanburgh, larsxschneider, miguel.torroja

Sure, I totally agree.
Sorry, I just wasn't clear enough in my previous email.
I meant that your patch suppresses "%s --> %s (%i MB)" line in case "fileSize" is not available,
while my patch suppresses just "(%i MB)" portion if the "fileSize" is not known.
In other words,
 * if "fileSize" is known:
 ** both yours and mine patches don't change existing behavior;
 * if "fileSize" is not known:
 ** your patch makes streamOneP4File() not print anything;
 ** my patch makes streamOneP4File() print "%s --> %s".

Hope, I'm clearer this time.
     
Thank you,
Andrey

From: Thandesha VK <thanvk@gmail.com>
> *I* think keeping the filesize info is better with --verbose option as
> that gives some clue about the file we are working on. What do you
> think?
> Script has similar checks of key existence at other places where it is
> looking for fileSize.
> 
> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo <amazo@checkvideo.com> wrote:
>> Huh, I actually have a slightly different fix for the same issue.
>> It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
>>
>> Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.
>>
>> Andrey Mazo (1):
>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>
>>  git-p4.py | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>>
>> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
>> --
>> 2.16.1
>>
> 
> -- 
> Thanks & Regards
> Thandesha VK | Cellphone +1 (703) 459-5386
^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-17 17:33     ` Mazo, Andrey
@ 2018-04-17 18:01       ` Thandesha VK
  2018-04-17 18:47         ` Mazo, Andrey
  0 siblings, 1 reply; 10+ messages in thread
From: Thandesha VK @ 2018-04-17 18:01 UTC (permalink / raw)
  To: Mazo, Andrey; +Cc: git, luke, gvanburgh, larsxschneider, miguel.torroja

Sounds good. How about an enhanced version of fix from both of us.
This will let us know that something is not right with the file but
will not bark

$ git diff
diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc6..df901976f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2566,7 +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'])
+            if 'fileSize' not in self.stream_file:
+               print "WARN: File size from perforce unknown. Please
verify by p4 sizes %s" %(file['depotFile'])
+               size = "-1"
+            else:
+               size = self.stream_file['fileSize']
+            size = int(size)
             sys.stdout.write('\r%s --> %s (%i MB)\n' %
(file['depotFile'], relPath, size/1024/1024))
             sys.stdout.flush()


On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
> Sure, I totally agree.
> Sorry, I just wasn't clear enough in my previous email.
> I meant that your patch suppresses "%s --> %s (%i MB)" line in case "fileSize" is not available,
> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not known.
> In other words,
>  * if "fileSize" is known:
>  ** both yours and mine patches don't change existing behavior;
>  * if "fileSize" is not known:
>  ** your patch makes streamOneP4File() not print anything;
>  ** my patch makes streamOneP4File() print "%s --> %s".
>
> Hope, I'm clearer this time.
>
> Thank you,
> Andrey
>
> From: Thandesha VK <thanvk@gmail.com>
>> *I* think keeping the filesize info is better with --verbose option as
>> that gives some clue about the file we are working on. What do you
>> think?
>> Script has similar checks of key existence at other places where it is
>> looking for fileSize.
>>
>> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo <amazo@checkvideo.com> wrote:
>>> Huh, I actually have a slightly different fix for the same issue.
>>> It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
>>>
>>> Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.
>>>
>>> Andrey Mazo (1):
>>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>>
>>>  git-p4.py | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>>
>>> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
>>> --
>>> 2.16.1
>>>
>>
>> --
>> Thanks & Regards
>> Thandesha VK | Cellphone +1 (703) 459-5386



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-17 18:01       ` Thandesha VK
@ 2018-04-17 18:47         ` Mazo, Andrey
  2018-04-17 19:12           ` Thandesha VK
  0 siblings, 1 reply; 10+ messages in thread
From: Mazo, Andrey @ 2018-04-17 18:47 UTC (permalink / raw)
  To: Thandesha VK; +Cc: git, luke, gvanburgh, larsxschneider, miguel.torroja

Does a missing "fileSize" actually mean that there is something wrong with the file?
Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
(which I attribute to our rather ancient (2007.2) Perforce server)
I'm not an expert in Perforce, so don't know for sure.

However, `p4 -G sizes` works fine even with our p4 server.
Should we then go one step further and use `p4 -G sizes` to obtain the "fileSize" when it's not returned by `p4 -G print`?
Or is it an overkill for a simple verbose print out?

Also, please, find one comment inline below.

Thank you,
Andrey

From: Thandesha VK <thanvk@gmail.com>
> Sounds good. How about an enhanced version of fix from both of us.
> This will let us know that something is not right with the file but
> will not bark
> 
> $ git diff
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc6..df901976f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2566,7 +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'])
> +            if 'fileSize' not in self.stream_file:
> +               print "WARN: File size from perforce unknown. Please verify by p4 sizes %s" %(file['depotFile'])
For whatever reason, the code below uses sys.stdout.write() instead of print().
Should it be used here for consistency as well?

> +               size = "-1"
> +            else:
> +               size = self.stream_file['fileSize']
> +            size = int(size)
>              sys.stdout.write('\r%s --> %s (%i MB)\n' %
> (file['depotFile'], relPath, size/1024/1024))
>              sys.stdout.flush()
> 
> 
> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
>> Sure, I totally agree.
>> Sorry, I just wasn't clear enough in my previous email.
>> I meant that your patch suppresses "%s --> %s (%i MB)" line in case "fileSize" is not available,
>> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not known.
>> In other words,
>>  * if "fileSize" is known:
>>  ** both yours and mine patches don't change existing behavior;
>>  * if "fileSize" is not known:
>>  ** your patch makes streamOneP4File() not print anything;
>>  ** my patch makes streamOneP4File() print "%s --> %s".
>>
>> Hope, I'm clearer this time.
>>
>> Thank you,
>> Andrey
>>
>> From: Thandesha VK <thanvk@gmail.com>
>>> *I* think keeping the filesize info is better with --verbose option as
>>> that gives some clue about the file we are working on. What do you
>>> think?
>>> Script has similar checks of key existence at other places where it is
>>> looking for fileSize.
>>>
>>> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo <amazo@checkvideo.com> wrote:
>>>> Huh, I actually have a slightly different fix for the same issue.
>>>> It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
>>>>
>>>> Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.
>>>>
>>>> Andrey Mazo (1):
>>>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>>>
>>>>  git-p4.py | 8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
>>>> --
>>>> 2.16.1
>>>>
>>>
>>> --
>>> Thanks & Regards
>>> Thandesha VK | Cellphone +1 (703) 459-5386
>
>
>
> -- 
> Thanks & Regards
> Thandesha VK | Cellphone +1 (703) 459-5386
^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-17 18:47         ` Mazo, Andrey
@ 2018-04-17 19:12           ` Thandesha VK
  2018-04-18 11:08             ` Luke Diamand
  0 siblings, 1 reply; 10+ messages in thread
From: Thandesha VK @ 2018-04-17 19:12 UTC (permalink / raw)
  To: Mazo, Andrey; +Cc: git, luke, gvanburgh, larsxschneider, miguel.torroja

I have few cases where even p4 -G sizes (or p4 sizes) is not returning
the size value even with latest version of p4 (17.2). In that case, we
have to regenerate the digest for file save it - It mean something is
wrong with the file in perforce.
Regarding, sys.stdout.write v/s print, I see script using both of them
without a common pattern. I can change it to whatever is more
appropriate.

On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
> Does a missing "fileSize" actually mean that there is something wrong with the file?
> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
> (which I attribute to our rather ancient (2007.2) Perforce server)
> I'm not an expert in Perforce, so don't know for sure.
>
> However, `p4 -G sizes` works fine even with our p4 server.
> Should we then go one step further and use `p4 -G sizes` to obtain the "fileSize" when it's not returned by `p4 -G print`?
> Or is it an overkill for a simple verbose print out?
>
> Also, please, find one comment inline below.
>
> Thank you,
> Andrey
>
> From: Thandesha VK <thanvk@gmail.com>
>> Sounds good. How about an enhanced version of fix from both of us.
>> This will let us know that something is not right with the file but
>> will not bark
>>
>> $ git diff
>> diff --git a/git-p4.py b/git-p4.py
>> index 7bb9cadc6..df901976f 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2566,7 +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'])
>> +            if 'fileSize' not in self.stream_file:
>> +               print "WARN: File size from perforce unknown. Please verify by p4 sizes %s" %(file['depotFile'])
> For whatever reason, the code below uses sys.stdout.write() instead of print().
> Should it be used here for consistency as well?
>
>> +               size = "-1"
>> +            else:
>> +               size = self.stream_file['fileSize']
>> +            size = int(size)
>>              sys.stdout.write('\r%s --> %s (%i MB)\n' %
>> (file['depotFile'], relPath, size/1024/1024))
>>              sys.stdout.flush()
>>
>>
>> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
>>> Sure, I totally agree.
>>> Sorry, I just wasn't clear enough in my previous email.
>>> I meant that your patch suppresses "%s --> %s (%i MB)" line in case "fileSize" is not available,
>>> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not known.
>>> In other words,
>>>  * if "fileSize" is known:
>>>  ** both yours and mine patches don't change existing behavior;
>>>  * if "fileSize" is not known:
>>>  ** your patch makes streamOneP4File() not print anything;
>>>  ** my patch makes streamOneP4File() print "%s --> %s".
>>>
>>> Hope, I'm clearer this time.
>>>
>>> Thank you,
>>> Andrey
>>>
>>> From: Thandesha VK <thanvk@gmail.com>
>>>> *I* think keeping the filesize info is better with --verbose option as
>>>> that gives some clue about the file we are working on. What do you
>>>> think?
>>>> Script has similar checks of key existence at other places where it is
>>>> looking for fileSize.
>>>>
>>>> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo <amazo@checkvideo.com> wrote:
>>>>> Huh, I actually have a slightly different fix for the same issue.
>>>>> It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
>>>>>
>>>>> Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.
>>>>>
>>>>> Andrey Mazo (1):
>>>>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>>>>
>>>>>  git-p4.py | 8 ++++++--
>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>>
>>>>> base-commit: 468165c1d8a442994a825f3684528361727cd8c0
>>>>> --
>>>>> 2.16.1
>>>>>
>>>>
>>>> --
>>>> Thanks & Regards
>>>> Thandesha VK | Cellphone +1 (703) 459-5386
>>
>>
>>
>> --
>> Thanks & Regards
>> Thandesha VK | Cellphone +1 (703) 459-5386



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-17 19:12           ` Thandesha VK
@ 2018-04-18 11:08             ` Luke Diamand
  2018-04-18 14:59               ` Thandesha VK
  0 siblings, 1 reply; 10+ messages in thread
From: Luke Diamand @ 2018-04-18 11:08 UTC (permalink / raw)
  To: Thandesha VK; +Cc: Mazo, Andrey, git, gvanburgh, larsxschneider, miguel.torroja

On 17 April 2018 at 20:12, Thandesha VK <thanvk@gmail.com> wrote:
> I have few cases where even p4 -G sizes (or p4 sizes) is not returning
> the size value even with latest version of p4 (17.2). In that case, we
> have to regenerate the digest for file save it - It mean something is
> wrong with the file in perforce.

Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.

We are just talking about the output from "p4 print" and the
"fileSize" key, right?

Does that happen with the 17.2 version of p4?

> Regarding, sys.stdout.write v/s print, I see script using both of them
> without a common pattern. I can change it to whatever is more
> appropriate.

print() probably makes more sense; can we try to use the function form
so that we don't deliberately make the path to python3 harder (albeit
in a very tiny way).

>
> On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
>> Does a missing "fileSize" actually mean that there is something wrong with the file?
>> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
>> (which I attribute to our rather ancient (2007.2) Perforce server)
>> I'm not an expert in Perforce, so don't know for sure.

My 2015 version of p4d reports a fileSize.

>>
>> However, `p4 -G sizes` works fine even with our p4 server.
>> Should we then go one step further and use `p4 -G sizes` to obtain the "fileSize" when it's not returned by `p4 -G print`?
>> Or is it an overkill for a simple verbose print out?

If your server isn't reporting "fileSize" then there are a few other
places where I would expect git-p4 to also fail.

If we're going to support this very ancient version of p4d, then
gracefully handling a missing fileSize will be useful.

>>
>> Also, please, find one comment inline below.
>>
>> Thank you,
>> Andrey
>>
>> From: Thandesha VK <thanvk@gmail.com>
>>> Sounds good. How about an enhanced version of fix from both of us.
>>> This will let us know that something is not right with the file but
>>> will not bark
>>>
>>> $ git diff
>>> diff --git a/git-p4.py b/git-p4.py
>>> index 7bb9cadc6..df901976f 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -2566,7 +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'])
>>> +            if 'fileSize' not in self.stream_file:
>>> +               print "WARN: File size from perforce unknown. Please verify by p4 sizes %s" %(file['depotFile'])
>> For whatever reason, the code below uses sys.stdout.write() instead of print().
>> Should it be used here for consistency as well?
>>
>>> +               size = "-1"
>>> +            else:
>>> +               size = self.stream_file['fileSize']
>>> +            size = int(size)
>>>              sys.stdout.write('\r%s --> %s (%i MB)\n' %
>>> (file['depotFile'], relPath, size/1024/1024))
>>>              sys.stdout.flush()
>>>
>>>
>>> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
>>>> Sure, I totally agree.
>>>> Sorry, I just wasn't clear enough in my previous email.
>>>> I meant that your patch suppresses "%s --> %s (%i MB)" line in case "fileSize" is not available,
>>>> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not known.
>>>> In other words,
>>>>  * if "fileSize" is known:
>>>>  ** both yours and mine patches don't change existing behavior;
>>>>  * if "fileSize" is not known:
>>>>  ** your patch makes streamOneP4File() not print anything;
>>>>  ** my patch makes streamOneP4File() print "%s --> %s".
>>>>
>>>> Hope, I'm clearer this time.
>>>>
>>>> Thank you,
>>>> Andrey
>>>>
>>>> From: Thandesha VK <thanvk@gmail.com>
>>>>> *I* think keeping the filesize info is better with --verbose option as
>>>>> that gives some clue about the file we are working on. What do you
>>>>> think?
>>>>> Script has similar checks of key existence at other places where it is
>>>>> looking for fileSize.
>>>>>
>>>>> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo <amazo@checkvideo.com> wrote:
>>>>>> Huh, I actually have a slightly different fix for the same issue.
>>>>>> It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
>>>>>>
>>>>>> Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.
>>>>>>
>>>>>> Andrey Mazo (1):
>>>>>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>>>>>
>>>>>>  git-p4.py | 8 ++++++--
>>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>>

Thanks
Luke

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key
  2018-04-18 11:08             ` Luke Diamand
@ 2018-04-18 14:59               ` Thandesha VK
  0 siblings, 0 replies; 10+ messages in thread
From: Thandesha VK @ 2018-04-18 14:59 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Mazo, Andrey, git, gvanburgh, larsxschneider, miguel.torroja

Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.

We are just talking about the output from "p4 print" and the
"fileSize" key, right?
--> Correct.

Does that happen with the 17.2 version of p4?
-->Correct.

print() probably makes more sense; can we try to use the function form
so that we don't deliberately make the path to python3 harder (albeit
in a very tiny way)
-->Sure.

If your server isn't reporting "fileSize" then there are a few other
places where I would expect git-p4 to also fail.
-->Most of other places are already doing key check in the hash. Looks
like this line was missed out.

On Wed, Apr 18, 2018 at 4:08 AM, Luke Diamand <luke@diamand.org> wrote:
> On 17 April 2018 at 20:12, Thandesha VK <thanvk@gmail.com> wrote:
>> I have few cases where even p4 -G sizes (or p4 sizes) is not returning
>> the size value even with latest version of p4 (17.2). In that case, we
>> have to regenerate the digest for file save it - It mean something is
>> wrong with the file in perforce.
>
> Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.
>
> We are just talking about the output from "p4 print" and the
> "fileSize" key, right?
>
> Does that happen with the 17.2 version of p4?
>
>> Regarding, sys.stdout.write v/s print, I see script using both of them
>> without a common pattern. I can change it to whatever is more
>> appropriate.
>
> print() probably makes more sense; can we try to use the function form
> so that we don't deliberately make the path to python3 harder (albeit
> in a very tiny way).
>
>>
>> On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
>>> Does a missing "fileSize" actually mean that there is something wrong with the file?
>>> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
>>> (which I attribute to our rather ancient (2007.2) Perforce server)
>>> I'm not an expert in Perforce, so don't know for sure.
>
> My 2015 version of p4d reports a fileSize.
>
>>>
>>> However, `p4 -G sizes` works fine even with our p4 server.
>>> Should we then go one step further and use `p4 -G sizes` to obtain the "fileSize" when it's not returned by `p4 -G print`?
>>> Or is it an overkill for a simple verbose print out?
>
> If your server isn't reporting "fileSize" then there are a few other
> places where I would expect git-p4 to also fail.
>
> If we're going to support this very ancient version of p4d, then
> gracefully handling a missing fileSize will be useful.
>
>>>
>>> Also, please, find one comment inline below.
>>>
>>> Thank you,
>>> Andrey
>>>
>>> From: Thandesha VK <thanvk@gmail.com>
>>>> Sounds good. How about an enhanced version of fix from both of us.
>>>> This will let us know that something is not right with the file but
>>>> will not bark
>>>>
>>>> $ git diff
>>>> diff --git a/git-p4.py b/git-p4.py
>>>> index 7bb9cadc6..df901976f 100755
>>>> --- a/git-p4.py
>>>> +++ b/git-p4.py
>>>> @@ -2566,7 +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'])
>>>> +            if 'fileSize' not in self.stream_file:
>>>> +               print "WARN: File size from perforce unknown. Please verify by p4 sizes %s" %(file['depotFile'])
>>> For whatever reason, the code below uses sys.stdout.write() instead of print().
>>> Should it be used here for consistency as well?
>>>
>>>> +               size = "-1"
>>>> +            else:
>>>> +               size = self.stream_file['fileSize']
>>>> +            size = int(size)
>>>>              sys.stdout.write('\r%s --> %s (%i MB)\n' %
>>>> (file['depotFile'], relPath, size/1024/1024))
>>>>              sys.stdout.flush()
>>>>
>>>>
>>>> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey <amazo@checkvideo.com> wrote:
>>>>> Sure, I totally agree.
>>>>> Sorry, I just wasn't clear enough in my previous email.
>>>>> I meant that your patch suppresses "%s --> %s (%i MB)" line in case "fileSize" is not available,
>>>>> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not known.
>>>>> In other words,
>>>>>  * if "fileSize" is known:
>>>>>  ** both yours and mine patches don't change existing behavior;
>>>>>  * if "fileSize" is not known:
>>>>>  ** your patch makes streamOneP4File() not print anything;
>>>>>  ** my patch makes streamOneP4File() print "%s --> %s".
>>>>>
>>>>> Hope, I'm clearer this time.
>>>>>
>>>>> Thank you,
>>>>> Andrey
>>>>>
>>>>> From: Thandesha VK <thanvk@gmail.com>
>>>>>> *I* think keeping the filesize info is better with --verbose option as
>>>>>> that gives some clue about the file we are working on. What do you
>>>>>> think?
>>>>>> Script has similar checks of key existence at other places where it is
>>>>>> looking for fileSize.
>>>>>>
>>>>>> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo <amazo@checkvideo.com> wrote:
>>>>>>> Huh, I actually have a slightly different fix for the same issue.
>>>>>>> It doesn't suppress the corresponding verbose output completely, but just removes the size information from it.
>>>>>>>
>>>>>>> Also, I'd mention that the workaround is trivial -- simply omit the "--verbose" option.
>>>>>>>
>>>>>>> Andrey Mazo (1):
>>>>>>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>>>>>>
>>>>>>>  git-p4.py | 8 ++++++--
>>>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>>>
>
> Thanks
> Luke



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-16 23:35 [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key Thandesha VK
2018-04-17 16:00 ` Mazo, Andrey
2018-04-17 16:22 ` Andrey Mazo
2018-04-17 17:21   ` Thandesha VK
2018-04-17 17:33     ` Mazo, Andrey
2018-04-17 18:01       ` Thandesha VK
2018-04-17 18:47         ` Mazo, Andrey
2018-04-17 19:12           ` Thandesha VK
2018-04-18 11:08             ` Luke Diamand
2018-04-18 14:59               ` Thandesha VK

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox