Re: [RFC/PATCHv2 2/2] git-p4: initial demonstration of possible RCS keyword fixup

From: Pete Wyckoff <pw@padd.com>
Date: Feb 11, 2012, 23:42 UTC
Message-ID: <20120211234248.GA16691@padd.com>
In-Reply-To: <1328829442-12550-3-git-send-email-luke@diamand.org>

luke@diamand.org wrote on Thu, 09 Feb 2012 23:17 +0000:

Show 8 quoted lines

> This change has a go at showing a possible way to fixup RCS
> keyword handling in git-p4.
> 
> It does not cope with deleted files.
> It does not have good test coverage.
> It does not solve the problem of the incorrect error messages.
> But it does at least work after a fashion, and could provide
> a starting point.

I think this has great promise. Since p4 can always be trusted to regenerate the keywords, we should be able to scrub them off at will.

I'm debating whether it's best just _always_ to scrub the files affected by a diff rather than trying the patch, checking for failure, then scrubbing and trying again.

I'll send along a bunch of test cases I wrote to play around with this. Your case had too many moving parts for me to understand. If there's something in there that isn't covered, maybe you can factor it out into something small? Feel free to merge any of my code in with a future resubmission.

Some comments in this code below:

Show 12 quoted lines

> @@ -753,6 +753,23 @@ class P4Submit(Command, P4UserMap):
>  
>          return result
>  
> +    def patchRCSKeywords(self, file):
> +        # Attempt to zap the RCS keywords in a p4 controlled file
> +        p4_edit(file)
> +        (handle, outFileName) = tempfile.mkstemp()
> +        outFile = os.fdopen(handle, "w+")
> +        inFile = open(file, "r")
> +        for line in inFile.readlines():
> +            line = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$',

> +                   r'$\1$', line)

I added a couple test cases that show how text+k, text+ko and everything else need to be handled differently.

Show 6 quoted lines

> +            outFile.write(line)
> +        inFile.close()
> +        outFile.close()
> +        # Forcibly overwrite the original file
> +        system("cat %s" % outFileName)
> +        system(["mv", "-f", outFileName, file])

Will need a good bit of error handling in the final version. I like the way you do it a line at a time, avoiding any issues with gigantic files.

Show 22 quoted lines

> @@ -964,9 +982,30 @@ class P4Submit(Command, P4UserMap):
>          patchcmd = diffcmd + " | git apply "
>          tryPatchCmd = patchcmd + "--check -"
>          applyPatchCmd = patchcmd + "--check --apply -"
> +        patch_succeeded = True
>  
>          if os.system(tryPatchCmd) != 0:
> +            fixed_rcs_keywords = False
> +            patch_succeeded = False
>              print "Unfortunately applying the change failed!"
> +
> +            # Patch failed, maybe it's just RCS keyword woes. Look through
> +            # the patch to see if that's possible.
> +            if gitConfig("git-p4.attemptRCSCleanup","--bool") == "true":
> +                file = None
> +                for line in read_pipe_lines(diffcmd):
> +                    m = re.match(r'^diff --git a/(.*)\s+b/(.*)', line)
> +                    if m:
> +                        file = m.group(1)
> +                    if re.match(r'.*\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$', line):
> +                        self.patchRCSKeywords(file)
> +                        fixed_rcs_keywords = True

This is a novel approach too. Instead of just guessing that keywords are causing the conflict, inspect the diff for context or edited lines containing keywords.

Or we could just always scrub every file before even trying to apply patches.

Show 15 quoted lines

> +            if fixed_rcs_keywords:
> +                print "Retrying the patch with RCS keywords cleaned up"
> +                if os.system(tryPatchCmd) == 0:
> +                    patch_succeeded = True
> +
> +        if not patch_succeeded:
>              print "What do you want to do?"
>              response = "x"
>              while response != "s" and response != "a" and response != "w":
> @@ -1588,11 +1627,11 @@ class P4Sync(Command, P4UserMap):
>          if type_base in ("text", "unicode", "binary"):
>              if "ko" in type_mods:
>                  text = ''.join(contents)
> -                text = re.sub(r'\$(Id|Header):[^$]*\$', r'$\1$', text)
> +                text = re.sub(r'\$(Id|Header)[^$]*\$', r'$\1$', text)

In a few spots I see you've taken the ":" out of the regex. This will match strings like $Idiot$ that shouldn't be keyword expanded.

Impressed.

		-- Pete