[PATCH 0/2] git-p4: Improvements to rename and copy detection

[PATCH 0/2] git-p4: Improvements to rename and copy detection

[Vitor Antunes]

The new version of these patches should, hopefully, contain all the corrections
that were discussing previously. Please check them and confirm everything.

In the meanwhile, I would kindly ask you to review the third patch I sent with
the subject "git-p4: Improve branch support". Regarding the specific point of
branches, I've had a discussion about branch origin detection in the thread
"git-p4: Corrected typo" to which I would like to have your opinion.

Thanks in advance for your time.

Vitor Antunes (2):
  git-p4: Add copy detection support
  git-p4: Improve branch support

 contrib/fast-import/git-p4 |   43 +++++++++++++++++++++++++++++++++++++++----
 1 files changed, 39 insertions(+), 4 deletions(-)

-- 
1.7.2.3

[PATCH 1/2] git-p4: Add copy detection support

[Vitor Antunes]

Add new config options:
    git-p4.detectCopies         - Enable copy detection.
    git-p4.detectCopiesHarder   - Find copies harder.
The detectCopies option should be set to a true/false value.
The detectCopiesHarder option should be set to true/false value.
P4Submit can now process diff-tree C status and integrate files accordingly.
---
 contrib/fast-import/git-p4 |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index b0da28a..1b1fc76 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -623,6 +623,12 @@ class P4Submit(Command):
         else:
             diffOpts = ""
 
+        if gitConfig("git-p4.detectCopies").lower() == "true":
+            diffOpts += " -C"
+
+        if gitConfig("git-p4.detectCopiesHarder").lower() == "true":
+            diffOpts += " --find-copies-harder"
+
         diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (diffOpts, id, id))
         filesToAdd = set()
         filesToDelete = set()
@@ -646,6 +652,16 @@ class P4Submit(Command):
                 filesToDelete.add(path)
                 if path in filesToAdd:
                     filesToAdd.remove(path)
+            elif modifier == "C":
+                src, dest = diff['src'], diff['dst']
+                p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+                if diff['src_sha1'] != diff['dst_sha1']:
+                    p4_system("edit \"%s\"" % (dest))
+                if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
+                    p4_system("edit \"%s\"" % (dest))
+                    filesToChangeExecBit[dest] = diff['dst_mode']
+                os.unlink(dest)
+                editedFiles.add(dest)
             elif modifier == "R":
                 src, dest = diff['src'], diff['dst']
                 p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
-- 
1.7.2.3

Re: [PATCH 1/2] git-p4: Add copy detection support

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000:
> Add new config options:
>     git-p4.detectCopies         - Enable copy detection.
>     git-p4.detectCopiesHarder   - Find copies harder.
> The detectCopies option should be set to a true/false value.
> The detectCopiesHarder option should be set to true/false value.
> P4Submit can now process diff-tree C status and integrate files accordingly.

Needs sign-off.
Acked-by: Pete Wyckoff <pw@padd.com>

[PATCH 2/2] git-p4: Improve branch support

[Vitor Antunes]

Add new config option branchUser to allow filtering P4 branch list by user.
Allow defining the branch list through branchList config option.
Correct base branch directory detection to use '/' as the split character.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 1b1fc76..93a0b6e 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -338,6 +338,11 @@ def gitConfig(key):
         _gitConfig[key] = read_pipe("git config %s" % key, ignore_error=True).strip()
     return _gitConfig[key]
 
+def gitConfigList(key):
+    if not _gitConfig.has_key(key):
+        _gitConfig[key] = read_pipe("git config --get-all %s" % key, ignore_error=True).strip().split(os.linesep)
+    return _gitConfig[key]
+
 def p4BranchesInGit(branchesAreInRemotes = True):
     branches = {}
 
@@ -1272,7 +1277,13 @@ class P4Sync(Command):
     def getBranchMapping(self):
         lostAndFoundBranches = set()
 
-        for info in p4CmdList("branches"):
+        user = gitConfig("git-p4.branchUser")
+        if len(user) > 0:
+            command = "branches -u %s" % user
+        else:
+            command = "branches"
+
+        for info in p4CmdList(command):
             details = p4Cmd("branch -o %s" % info["branch"])
             viewIdx = 0
             while details.has_key("View%s" % viewIdx):
@@ -1305,6 +1316,12 @@ class P4Sync(Command):
         for branch in lostAndFoundBranches:
             self.knownBranches[branch] = branch
 
+        configBranches = gitConfigList("git-p4.branchList")
+        for branch in configBranches:
+            if branch:
+                (source, destination) = branch.split(":")
+                self.knownBranches[destination] = source
+
     def getBranchMappingFromGitBranches(self):
         branches = p4BranchesInGit(self.importIntoRemotes)
         for branch in branches.keys():
@@ -1626,12 +1643,14 @@ class P4Sync(Command):
                     else:
                         paths = []
                         for (prev, cur) in zip(self.previousDepotPaths, depotPaths):
-                            for i in range(0, min(len(cur), len(prev))):
-                                if cur[i] <> prev[i]:
+                            prev_list = prev.split("/")
+                            cur_list = cur.split("/")
+                            for i in range(0, min(len(cur_list), len(prev_list))):
+                                if cur_list[i] <> prev_list[i]:
                                     i = i - 1
                                     break
 
-                            paths.append (cur[:i + 1])
+                            paths.append ("/".join(cur_list[:i + 1]))
 
                         self.previousDepotPaths = paths
 
-- 
1.7.2.3

Re: [PATCH 2/2] git-p4: Improve branch support

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000:
> Add new config option branchUser to allow filtering P4 branch list by user.
> Allow defining the branch list through branchList config option.
> Correct base branch directory detection to use '/' as the split character.

I've only been avoiding reading this one because I don't use
branches, and am having a hard time following what the point
of the change is.  Maybe someone else will notice that this is
a good change straightaway, and save you the bother of explaining
it to me.

The whole detectBranches option seems a bit spotty, given its
lack of documentation and numerous shady "## HACK" and "## FIXME"
comments.  So it certainly does appreciate your attention.

As far as I can tell, a branch is just a mapping between two
areas of the repo, and all you can do with one is to feed it
to "p4 integrate" instead of giving a single explicit src/dest.

> @@ -1272,7 +1277,13 @@ class P4Sync(Command):
>      def getBranchMapping(self):
>          lostAndFoundBranches = set()
>  
> -        for info in p4CmdList("branches"):
> +        user = gitConfig("git-p4.branchUser")
> +        if len(user) > 0:
> +            command = "branches -u %s" % user
> +        else:
> +            command = "branches"
> +
> +        for info in p4CmdList(command):

This part is for branches -u, to limit the auto-detected braches
to just those created by a given user.

> @@ -1305,6 +1316,12 @@ class P4Sync(Command):
>          for branch in lostAndFoundBranches:
>              self.knownBranches[branch] = branch
>  
> +        configBranches = gitConfigList("git-p4.branchList")
> +        for branch in configBranches:
> +            if branch:
> +                (source, destination) = branch.split(":")
> +                self.knownBranches[destination] = source
> +

This bit adds more branches, if you have put them in .git/config
separated by a :.  Presumably things that are directories in the
depot but do _not_ have a branch?  I don't get it.  What does
your depot look like and what do you stick in branchList?

>          branches = p4BranchesInGit(self.importIntoRemotes)
>          for branch in branches.keys():
> @@ -1626,12 +1643,14 @@ class P4Sync(Command):
>                      else:
>                          paths = []
>                          for (prev, cur) in zip(self.previousDepotPaths, depotPaths):
> -                            for i in range(0, min(len(cur), len(prev))):
> -                                if cur[i] <> prev[i]:
> +                            prev_list = prev.split("/")
> +                            cur_list = cur.split("/")
> +                            for i in range(0, min(len(cur_list), len(prev_list))):
> +                                if cur_list[i] <> prev_list[i]:
>                                      i = i - 1
>                                      break
>  
> -                            paths.append (cur[:i + 1])
> +                            paths.append ("/".join(cur_list[:i + 1]))

This was a bug?  I guess somehow this is adapting the existing
depot paths to include new ones that you discovered with "p4
branches".  And now you are comparing the path elements instead
of going a character at a time.  Maybe to catch //depot/ab vs
//depot/ac ?

In other words, add some comments or add something to the commit
text.  It is encouraged to split up a patch into the smallest
logical chenk if the changes are all independent.  Maybe these
three are related through the theme of auto-branch detection.

		-- Pete

Re: [PATCH 2/2] git-p4: Improve branch support

[Vitor Antunes]

Hi Pete,

First thing I just noticed is that I wanted to send the "Improve
rename detection" patch and not this one.
And now, after I sent it I understood that I still don't know how to
use the format-patch correctly because I sent it as a 1/3 set of
patches... :/
Sorry to all the mailing list about this.

On Thu, Feb 17, 2011 at 6:42 PM, Pete Wyckoff <pw@padd.com> wrote:
> vitor.hda@gmail.com wrote on Tue, 15 Feb 2011 23:49 +0000:
>> Add new config option branchUser to allow filtering P4 branch list by user.
>> Allow defining the branch list through branchList config option.
>> Correct base branch directory detection to use '/' as the split character.
>
> I've only been avoiding reading this one because I don't use
> branches, and am having a hard time following what the point
> of the change is.  Maybe someone else will notice that this is
> a good change straightaway, and save you the bother of explaining
> it to me.
>
> The whole detectBranches option seems a bit spotty, given its
> lack of documentation and numerous shady "## HACK" and "## FIXME"
> comments.  So it certainly does appreciate your attention.

I also noticed those comments in the code, but until now I did not
find any wrong behavior on how the script works with branches. But
then again, in our depot we don't do many complex operations.
I still want to add an improvement on how it detects the branch's
origin commit. If while doing that I see something else I can improve
I will do so :)

> As far as I can tell, a branch is just a mapping between two
> areas of the repo, and all you can do with one is to feed it
> to "p4 integrate" instead of giving a single explicit src/dest.

In P4 branches work like in Subversion - they are simple links to the
origin data, which are created with the integrate command. But we can
also define a branch specification. That way we can simply refer to
that branch for the integrate to know where to update the data
from/to. git-p4 was using the branch specs to search for branches.

>> @@ -1272,7 +1277,13 @@ class P4Sync(Command):
>>      def getBranchMapping(self):
>>          lostAndFoundBranches = set()
>>
>> -        for info in p4CmdList("branches"):
>> +        user = gitConfig("git-p4.branchUser")
>> +        if len(user) > 0:
>> +            command = "branches -u %s" % user
>> +        else:
>> +            command = "branches"
>> +
>> +        for info in p4CmdList(command):
>
> This part is for branches -u, to limit the auto-detected braches
> to just those created by a given user.

Yes, as simple as that!
When you have a worldwide server an ocean away with thousands of
branch specifications, you really don't want to go through all of
them... ;)

>> @@ -1305,6 +1316,12 @@ class P4Sync(Command):
>>          for branch in lostAndFoundBranches:
>>              self.knownBranches[branch] = branch
>>
>> +        configBranches = gitConfigList("git-p4.branchList")
>> +        for branch in configBranches:
>> +            if branch:
>> +                (source, destination) = branch.split(":")
>> +                self.knownBranches[destination] = source
>> +
>
> This bit adds more branches, if you have put them in .git/config
> separated by a :.  Presumably things that are directories in the
> depot but do _not_ have a branch?  I don't get it.  What does
> your depot look like and what do you stick in branchList?

Assume

//depot/project

as the main branch. And

//depot/project_featureA
//depot/project_featureB

as feature development branches. So, you could have a list like:
project:project_featureA
project:project_featureB

Of course, if project_featureB originated from the first dev branch,
you would need to define it like:

project_featureA:project_featureB

This was to avoid using P4 branch specifications. In my team we don't
use them that much and since I was only creating branch specs to use
git-p4, I kinda of thought it would be better do keep that
configuration within git.

>>          branches = p4BranchesInGit(self.importIntoRemotes)
>>          for branch in branches.keys():
>> @@ -1626,12 +1643,14 @@ class P4Sync(Command):
>>                      else:
>>                          paths = []
>>                          for (prev, cur) in zip(self.previousDepotPaths, depotPaths):
>> -                            for i in range(0, min(len(cur), len(prev))):
>> -                                if cur[i] <> prev[i]:
>> +                            prev_list = prev.split("/")
>> +                            cur_list = cur.split("/")
>> +                            for i in range(0, min(len(cur_list), len(prev_list))):
>> +                                if cur_list[i] <> prev_list[i]:
>>                                      i = i - 1
>>                                      break
>>
>> -                            paths.append (cur[:i + 1])
>> +                            paths.append ("/".join(cur_list[:i + 1]))
>
> This was a bug?  I guess somehow this is adapting the existing
> depot paths to include new ones that you discovered with "p4
> branches".  And now you are comparing the path elements instead
> of going a character at a time.  Maybe to catch //depot/ab vs
> //depot/ac ?

Exactly. The example I used above was created to also show that.

> In other words, add some comments or add something to the commit
> text.  It is encouraged to split up a patch into the smallest
> logical chenk if the changes are all independent.  Maybe these
> three are related through the theme of auto-branch detection.

Understood. I think this patch can easily be split into 3 different
logical chunks.
I'll add some comments and probably see if I can include some
descriptions/examples in git-p4.txt. I just have to find some free
time first :)

Thank you very much for your feedback and help.

-- 
Vitor Antunes