[PATCH] Fix git p4 sync errors

[PATCH] Fix git p4 sync errors

[Matt Arsenault]

>From 425e4dc6992d07aa00039c5bb8e8c76def591fd3 Mon Sep 17 00:00:00 2001
From: Matt Arsenault <arsenm2@gmail.com>
Date: Sat, 20 Oct 2012 18:48:45 -0700
Subject: [PATCH] git-p4: Fix not using -s option to describe

This solves errors in some cases when syncing renamed files.
Other places where describe is used use the -s, except this one.

Signed-off-by: Matt Arsenault <arsenm2@gmail.com>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 882b1bb..e203508 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2543,7 +2543,7 @@ class P4Sync(Command, P4UserMap):
     def importChanges(self, changes):
         cnt = 1
         for change in changes:
-            description = p4Cmd(["describe", str(change)])
+            description = p4Cmd(["describe", "-s", str(change)])
             self.updateOptionDict(description)
 
             if not self.silent:
-- 
1.7.12.2

Re: [PATCH] Fix git p4 sync errors

[Junio C Hamano]

[Added area experts of git-p4 to Cc.]

Matt Arsenault <arsenm2@gmail.com> writes:

> From 425e4dc6992d07aa00039c5bb8e8c76def591fd3 Mon Sep 17 00:00:00 2001
> From: Matt Arsenault <arsenm2@gmail.com>
> Date: Sat, 20 Oct 2012 18:48:45 -0700
> Subject: [PATCH] git-p4: Fix not using -s option to describe

Please do not include these four lines in the body of the message.
The "Subject: " line is there so that you can copy & paste to your
MUA (instead of having to type something different from scratch),
and we use the authorship From/Date from your e-mail message.

>
> This solves errors in some cases when syncing renamed files.

Can you be a bit more descriptive?  What are "errors in some case"?

A good rule of thumb is to imagine a user of this command who is
having a problem who reads this commit log message.  Does the
symptom described to have fixed by the commit detailed enough to
allow her to tell if this change may potentially fix the problem she
is having?

> Other places where describe is used use the -s, except this one.

This makes it sound as if the changed line was an odd-man out among
many others, and it is clear we always want "describe -s" throughout
this command.  But my "git grep 'p4.*describe'" shows there are two
places, one with "-s" and one without.

What are returned from this invocation of p4Cmd() seem to be passed
to self.splitFilesIntoBranches() as its commit[] and the fields
'depotFile#', 'rev#', 'action#' and 'type#' are looked at in the
method, and then to self.commit() as its details[], for fields
'time', 'user', 'change', 'desc', and 'options'.

What field, if any, gets wrong value when "-s" is not used?  Or
perhaps some fields are omitted incorrectly?  What field are we
getting out of p4Cmd() that we are not using in the existing
callchain by not passing "-s" [*1*]?

In short, what I am getting at are:

 - What breaks by not passing "-s"?  What are the user visible
   symptoms?

 - Why is it a bug not to pass "-s"?  How does the bug happen?

Thanks.

[footnote]

*1*

http://www.perforce.com/perforce/r12.1/manuals/cmdref/describe.html
says that "describe -s" omits files diffs, but I do not know how it
affects "-G describe -s" output that we are reading (what fields in
the unmarshalled dict we are reading from is affected).

> Signed-off-by: Matt Arsenault <arsenm2@gmail.com>
> ---
>  git-p4.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 882b1bb..e203508 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2543,7 +2543,7 @@ class P4Sync(Command, P4UserMap):
>      def importChanges(self, changes):
>          cnt = 1
>          for change in changes:
> -            description = p4Cmd(["describe", str(change)])
> +            description = p4Cmd(["describe", "-s", str(change)])
>              self.updateOptionDict(description)
>  
>              if not self.silent:

Re: [PATCH] Fix git p4 sync errors

[Matt Arsenault]

On Oct 21, 2012, at 12:06 , Junio C Hamano <gitster@pobox.com> wrote:
> 
>> 
>> This solves errors in some cases when syncing renamed files.
> 
> Can you be a bit more descriptive?  What are "errors in some case"?
> 
It might just be when files are renamed. I ran into this after months of using it, and I'm skeptical that in that time no files were ever renamed. I'm not sure what was special about the file that was renamed. (There also might have been deleted files in the same commit, not sure if that matters)

> In short, what I am getting at are:
> 
> - What breaks by not passing "-s"?  What are the user visible
>   symptoms?

There's a key error on the line
line 2198:        epoch = details["time"]
The details object is an error different fields set (I don't remember what it is exactly, I'm not at work right now)

> 
> - Why is it a bug not to pass "-s"?  How does the bug happen?

I encountered this one time after using it for months. One day I couldn't git p4 rebase with the key error.  I searched for the error and found some version of git-p4 that fixed a similar error by adding the -s to describe. Adding the -s fixed the error and everything seemed to be working correctly.

Re: [PATCH] Fix git p4 sync errors

[Christian Couder]

Hi,

On Thu, Oct 25, 2012 at 4:41 AM, Matt Arsenault <arsenm2@gmail.com> wrote:
>
> On Oct 21, 2012, at 12:06 , Junio C Hamano <gitster@pobox.com> wrote:
>>
>> - Why is it a bug not to pass "-s"?  How does the bug happen?
>
> I encountered this one time after using it for months. One day I couldn't git p4 rebase
> with the key error.  I searched for the error and found some version of git-p4 that fixed
> a similar error by adding the -s to describe. Adding the -s fixed the error and
> everything seemed to be working correctly.

The perforce documentation says the following about this flag:

"The -s flag omits the diffs of files that were updated."

So if the diffs are not used, there is no downside to use -s.
Maybe the patch should just state this.

Best,
Christian.

Re: [PATCH] Fix git p4 sync errors

[Pete Wyckoff]

arsenm2@gmail.com wrote on Wed, 24 Oct 2012 19:41 -0700:
> 
> On Oct 21, 2012, at 12:06 , Junio C Hamano <gitster@pobox.com> wrote:
> > 
> >> 
> >> This solves errors in some cases when syncing renamed files.
> > 
> > Can you be a bit more descriptive?  What are "errors in some case"?
> > 
> It might just be when files are renamed. I ran into this after months of using it, and I'm skeptical that in that time no files were ever renamed. I'm not sure what was special about the file that was renamed. (There also might have been deleted files in the same commit, not sure if that matters)

I set up a test case where I did a "p4 move" on a file and tried
syncing it, with and without "-s" to describe.  It works in both
cases, for an old (2009.2) and new (2012.1) version of p4.

The output of -s versus no -s does differ, and the differences
are different with server version worse yet.  But in no case is
there ever a set of file differences.  -G does seem to disable
that.

I'd love to track this down, but can't seem to provoke anything
on my own.  Let me know if you have any hints based on what is in
your depot or server/client config.  Or if you see it again.

> > In short, what I am getting at are:
> > 
> > - What breaks by not passing "-s"?  What are the user visible
> >   symptoms?
> 
> There's a key error on the line
> line 2198:        epoch = details["time"]
> The details object is an error different fields set (I don't remember what it is exactly, I'm not at work right now)

This would happen if describe did not return a "time" field, but
there's an explicit check for that:

        res = p4CmdList("describe -s %d" % newestRevision)
        newestTime = None   
        for r in res:       
            if r.has_key('time'):
                newestTime = int(r['time'])
        if newestTime is None:
            die("Output from \"describe -s\" on newest change %d did not give a time" %                     
                newestRevision) 
        details["time"] = newestTime

so I'm confused how this could happen.  Maybe your version is
older/different than what is in the git source?


I'm not against putting in your patch, since it is true we don't
want the file diff, and adding "-s" should be harmless in theory.
And it doesn't cause any existing tests to fail.  It just scares
me that there's something else going on we haven't figured out.

		-- Pete