threads / patch / 26367

patch, 2 partsgit-p4: Improve rename detection support.

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

## tl;dr

10 messages between Jan 30, 2011 and Feb 12, 2011. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Vitor Antunes· Jan 30, 2011, 23:19 UTC · lore

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 <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

Show changes to contrib/fast-import/git-p4 +9 −2

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

Vitor Antunes· Jan 30, 2011, 23:19 UTC · re: Vitor Antunes · lore

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

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(-)

Show changes to contrib/fast-import/git-p4 +17 −0

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

Pete Wyckoff· Feb 6, 2011, 00:25 UTC · re: Vitor Antunes · lore

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

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

Show 7 quoted lines

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

[..]

Show 8 quoted lines

> +        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.

Show 9 quoted lines

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

Vitor Antunes· Feb 6, 2011, 17:25 UTC · re: Pete Wyckoff · lore

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

Hi Pete,

On Sun, Feb 6, 2011 at 12:25 AM, Pete Wyckoff <pw@padd.com> 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

Pete Wyckoff· Feb 6, 2011, 22:05 UTC · re: Vitor Antunes · lore

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

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

Show 17 quoted lines

> Hi Pete,
> 
> On Sun, Feb 6, 2011 at 12:25 AM, Pete Wyckoff <pw@padd.com> 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?

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

Show 9 quoted lines

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

Vitor Antunes· Feb 7, 2011, 11:11 UTC · re: Pete Wyckoff · lore

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

Hi Pete,

On Sun, Feb 6, 2011 at 10:05 PM, Pete Wyckoff <pw@padd.com> wrote:

Show 14 quoted lines

>> (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.

Show 14 quoted lines

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

Vitor Antunes· Feb 12, 2011, 00:29 UTC · re: Vitor Antunes · lore

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

Hi Pete,

On Mon, Feb 7, 2011 at 11:11 AM, Vitor Antunes <vitor.hda@gmail.com> wrote:

Show 41 quoted lines

> Hi Pete,
>
> On Sun, Feb 6, 2011 at 10:05 PM, Pete Wyckoff <pw@padd.com> wrote:
>>> (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.
>
>>> +            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

Pete Wyckoff· Feb 12, 2011, 13:32 UTC · re: Vitor Antunes · lore

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

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

Show 8 quoted lines

> 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

Pete Wyckoff· Feb 6, 2011, 00:21 UTC · re: Vitor Antunes · lore

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

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.

Show 9 quoted lines

>      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 = ""

Show 13 quoted lines

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

		-- Pete

Vitor Antunes· Feb 6, 2011, 12:39 UTC · re: Pete Wyckoff · lore

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

Hi Pete,

On Sun, Feb 6, 2011 at 12:21 AM, Pete Wyckoff <pw@padd.com> 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...

Show 13 quoted lines

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

Show 16 quoted lines

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