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

[Pete Wyckoff <pw@padd.com>]

luke@diamand.org wrote on Thu, 09 Feb 2012 23:17 +0000:
> 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:

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

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

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

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