Re: [PATCH] Fix git p4 sync errors

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2012, 19:06 UTC
Message-ID: <7vwqyjfxwd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <AC43C2B4-623F-4590-9F92-6CCA26645EFE@gmail.com>

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

Show 18 quoted lines

> 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: