From: Vitor Antunes <vitor.hda@gmail.com>
Date: Sun, 06 Feb 2011 12:39:25 GMT
Subject: Re: [PATCH 1/2] git-p4: Improve rename detection support.
Message-ID: <AANLkTikd+rGpfy9x35SA0g4F8uYpjp_+GK4m9pagu8Nn@mail.gmail.com>
In-Reply-To: <20110206002113.GA31245@arf.padd.com>

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

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

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