[PATCH 1/2] git-p4: Improve rename detection support.

Only open files for edit after integrating if the SHA1 of source and destination
differ from each other.
Add git config option detectRenames to allow permanent rename detection. This
options should be set to a true/false value.

Signed-off-by: Vitor Antunes <redacted>
---
 contrib/fast-import/git-p4 |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 04ce7e3..9fb480a 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4

@@ -613,7 +613,13 @@ class P4Submit(Command):
 
     def applyCommit(self, id):
         print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
-        diffOpts = ("", "-M")[self.detectRename]
+
+        detectRenames = gitConfig("git-p4.detectRenames")
+        if len(detectRenames) and detectRenames.lower() != "false" > 0:
+            diffOpts = "-M"
+        else:
+            diffOpts = ("", "-M")[self.detectRename]
+
         diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (diffOpts, id, id))
         filesToAdd = set()
         filesToDelete = set()

@@ -640,7 +646,8 @@ class P4Submit(Command):
             elif modifier == "R":
                 src, dest = diff['src'], diff['dst']
                 p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
-                p4_system("edit \"%s\"" % (dest))
+                if diff['src_sha1'] != diff['dst_sha1']:
+                    p4_system("edit \"%s\"" % (dest))
                 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
                     filesToChangeExecBit[dest] = diff['dst_mode']
                 os.unlink(dest)

-- 
1.7.2.3

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 |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 9fb480a..9b67ae2 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4

@@ -620,6 +620,14 @@ class P4Submit(Command):
         else:
             diffOpts = ("", "-M")[self.detectRename]
 
+        detectCopies = gitConfig("git-p4.detectCopies")
+        if len(detectCopies) and detectCopies.lower() != "false" > 0:
+            diffOpts += " -C"
+
+        detectCopiesHarder = gitConfig("git-p4.detectCopiesHarder")
+        if len(detectCopiesHarder) > 0 and detectCopiesHarder.lower() != "false":
+            diffOpts += " --find-copies-harder"
+
         diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (diffOpts, id, id))
         filesToAdd = set()
         filesToDelete = set()

@@ -643,6 +651,15 @@ 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']):
+                    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

vitor.hda@gmail.com wrote on Sun, 30 Jan 2011 23:19 +0000:

Only open files for edit after integrating if the SHA1 of source and destination
differ from each other.
Add git config option detectRenames to allow permanent rename detection. This
options should be set to a true/false value.

I like the idea.

     def applyCommit(self, id):
         print "Applying %s" % (read_pipe("git log --max-count=1 --pretty=oneline %s" % id))
-        diffOpts = ("", "-M")[self.detectRename]
+
+        detectRenames = gitConfig("git-p4.detectRenames")
+        if len(detectRenames) and detectRenames.lower() != "false" > 0:
+            diffOpts = "-M"
+        else:
+            diffOpts = ("", "-M")[self.detectRename]

The comparisons confuse me.  detectRenames != "false" > 0  ?
How about just detectRenames == "true"?

You could rename the existing self.detectRename to add an "s" so
it all makes a bit more sense.

    if not self.detectRenames:
	# not explicitly set, check the config variable
	b = gitConfig("git-p4.detectRenames")
	if b == "true":
	    self.detectRenames = "-M"

    if self.detectRenames:
	diffOpts = "-M"
    else:
	diffOpts = ""

If you rename the file and also cause its perms to change (say
add +x), does it still work if dest is not open?

		-- Pete

vitor.hda@gmail.com wrote on Sun, 30 Jan 2011 23:19 +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.
---

[..]

+        detectCopies = gitConfig("git-p4.detectCopies")
+        if len(detectCopies) and detectCopies.lower() != "false" > 0:
+            diffOpts += " -C"
+
+        detectCopiesHarder = gitConfig("git-p4.detectCopiesHarder")
+        if len(detectCopiesHarder) > 0 and detectCopiesHarder.lower() != "false":
+            diffOpts += " --find-copies-harder"
+

I like it, but same weirdness with != > 0.

+            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']):
+                    filesToChangeExecBit[dest] = diff['dst_mode']
+                os.unlink(dest)
+                editedFiles.add(dest)

You can use integrate -t to force the filetype even if the file
already existed, and skip the whole execbit change.

		-- Pete

Hi Pete,

On Sun, Feb 6, 2011 at 12:25 AM, Pete Wyckoff [off-list ref] wrote:

You can use integrate -t to force the filetype even if the file
already existed, and skip the whole execbit change.

(Copying help text:
	The -t flag makes the source file's filetype propagate to the target
	file.  Normally, the target file retains its previous filetype.
	Newly branched files always use the source file's filetype.  The
	filetype can still be changed before 'p4 submit' with 'p4 reopen'.
)

Since in git we're only considering newly branched files, I think in
this case "-t" will not add anything. In fact, what is being done here
is detecting exec bit changes from source to target files - we're not
trying to force P4 to use the source's exec bit. Do you agree?

Kind regards,
-- 
Vitor Antunes

Hi Pete,

On Sun, Feb 6, 2011 at 12:21 AM, Pete Wyckoff [off-list ref] wrote:

The comparisons confuse me.  detectRenames != "false" > 0  ?
How about just detectRenames == "true"?

The "> 0" was for the length check. I somehow (*feeling embarrassed*)
misplaced that code...

You could rename the existing self.detectRename to add an "s" so
it all makes a bit more sense.

   if not self.detectRenames:
       # not explicitly set, check the config variable
       b = gitConfig("git-p4.detectRenames")
       if b == "true":
           self.detectRenames = "-M"

   if self.detectRenames:
       diffOpts = "-M"
   else:
       diffOpts = ""

Seems like a better idea. I kind of like the original code to set
diffOpts, so I would prefer to keep it. What do you think of (didn't
test it):

        if not self.detectRenames:
            # If not explicitly set check the config variable
            self.detectRenames =
gitConfig("git-p4.detectRenames").lower() == "true"

        diffOpts = ("", "-M")[self.detectRenames]

quoted

         diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (diffOpts, id, id))
         filesToAdd = set()
         filesToDelete = set()

@@ -640,7 +646,8 @@ class P4Submit(Command):

             elif modifier == "R":
                 src, dest = diff['src'], diff['dst']
                 p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
-                p4_system("edit \"%s\"" % (dest))
+                if diff['src_sha1'] != diff['dst_sha1']:
+                    p4_system("edit \"%s\"" % (dest))
                 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
                     filesToChangeExecBit[dest] = diff['dst_mode']
                 os.unlink(dest)

If you rename the file and also cause its perms to change (say
add +x), does it still work if dest is not open?

This is a very good point. I will also open the file for edit when
there is a mode change.

Thanks!

-- 
Vitor Antunes

vitor.hda@gmail.com wrote on Sun, 06 Feb 2011 17:25 +0000:

Hi Pete,

On Sun, Feb 6, 2011 at 12:25 AM, Pete Wyckoff [off-list ref] wrote:

quoted

You can use integrate -t to force the filetype even if the file
already existed, and skip the whole execbit change.

(Copying help text:
	The -t flag makes the source file's filetype propagate to the target
	file.  Normally, the target file retains its previous filetype.
	Newly branched files always use the source file's filetype.  The
	filetype can still be changed before 'p4 submit' with 'p4 reopen'.
)

Since in git we're only considering newly branched files, I think in
this case "-t" will not add anything. In fact, what is being done here
is detecting exec bit changes from source to target files - we're not
trying to force P4 to use the source's exec bit. Do you agree?

That sounds fine to me.  The code seemed to indicate that
sometimes the destination file exists.

+            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']):
+                    filesToChangeExecBit[dest] = diff['dst_mode']
+                os.unlink(dest)
+                editedFiles.add(dest)

If you're happy the dest never exists, you may be able to get rid
of the edit step and the mode-change check entirely.  As long as
you've tested this, you're the expert here.  The change makes
sense overall.

		-- Pete

Hi Pete,

On Sun, Feb 6, 2011 at 10:05 PM, Pete Wyckoff [off-list ref] wrote:

quoted

(Copying help text:
      The -t flag makes the source file's filetype propagate to the target
      file.  Normally, the target file retains its previous filetype.
      Newly branched files always use the source file's filetype.  The
      filetype can still be changed before 'p4 submit' with 'p4 reopen'.
)

Since in git we're only considering newly branched files, I think in
this case "-t" will not add anything. In fact, what is being done here
is detecting exec bit changes from source to target files - we're not
trying to force P4 to use the source's exec bit. Do you agree?

That sounds fine to me.  The code seemed to indicate that
sometimes the destination file exists.

I've basically copied the code from the rename detection part and
adapted it to copying. Nevertheless, even if git detects a copy to a
already existing file I think that the integrate command should behave
correctly. I should create a test case for this, though.

quoted

+            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']):
+                    filesToChangeExecBit[dest] = diff['dst_mode']
+                os.unlink(dest)
+                editedFiles.add(dest)

If you're happy the dest never exists, you may be able to get rid
of the edit step and the mode-change check entirely.  As long as
you've tested this, you're the expert here.  The change makes
sense overall.

I'm not that happy with this... and I'm no expert! I will really need
to test this possibility. Just need to understand how I can make git
detect a copy to an existing file... :)

Thanks for your feedback,
-- 
Vitor Antunes

Hi Pete,

On Mon, Feb 7, 2011 at 11:11 AM, Vitor Antunes [off-list ref] wrote:

Hi Pete,

On Sun, Feb 6, 2011 at 10:05 PM, Pete Wyckoff [off-list ref] wrote:

quoted

quoted

(Copying help text:
      The -t flag makes the source file's filetype propagate to the target
      file.  Normally, the target file retains its previous filetype.
      Newly branched files always use the source file's filetype.  The
      filetype can still be changed before 'p4 submit' with 'p4 reopen'.
)

Since in git we're only considering newly branched files, I think in
this case "-t" will not add anything. In fact, what is being done here
is detecting exec bit changes from source to target files - we're not
trying to force P4 to use the source's exec bit. Do you agree?

That sounds fine to me.  The code seemed to indicate that
sometimes the destination file exists.

I've basically copied the code from the rename detection part and
adapted it to copying. Nevertheless, even if git detects a copy to a
already existing file I think that the integrate command should behave
correctly. I should create a test case for this, though.

quoted

quoted

+            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']):
+                    filesToChangeExecBit[dest] = diff['dst_mode']
+                os.unlink(dest)
+                editedFiles.add(dest)

If you're happy the dest never exists, you may be able to get rid
of the edit step and the mode-change check entirely.  As long as
you've tested this, you're the expert here.  The change makes
sense overall.

I'm not that happy with this... and I'm no expert! I will really need
to test this possibility. Just need to understand how I can make git
detect a copy to an existing file... :)

I've tried to trick git into detecting a copy over an existing file by
replacing a file with the contents of another file. But git is not
detecting this as a copy. How can I make git detect a copy over an
existing file?

After re-reading your answer I got the feeling that this SHA diff code
may have been misinterpreted. So, just in case it was, here is the
full history.
Originally the code always opened the files for editing just in case
there was a patch to be applied late,r. This makes sense, because git
can detect a rename (or copy) even if the files differ. But doing this
would create an "unclean" integration in the revision graph. So, what
I thought was "if we have available the SHA of the origin and
destination files in diff-tree output why not compare them before
opening the file for editing?" And that was the only purpose for this
comparison.

-- 
Vitor Antunes

vitor.hda@gmail.com wrote on Sat, 12 Feb 2011 00:29 +0000:

Originally the code always opened the files for editing just in case
there was a patch to be applied late,r. This makes sense, because git
can detect a rename (or copy) even if the files differ. But doing this
would create an "unclean" integration in the revision graph. So, what
I thought was "if we have available the SHA of the origin and
destination files in diff-tree output why not compare them before
opening the file for editing?" And that was the only purpose for this
comparison.

I get what you are saying and it makes sense to fix this.  I
think you are on the right track, and I can't see the problem
in the code.

Side note:  p4 installations can make client SubmitOptions
have "revertunchanged" to avoid these annoying null integrations.

		-- Pete