[RFC/PATCHv2 0/2] git-p4: possible RCS keyword fixes

[RFC/PATCHv2 0/2] git-p4: possible RCS keyword fixes

[Luke Diamand]

Re-rolled with elementary shell coding errors fixed as per comments
from Junio - thanks!

I said before I wouldn't try to fix this, but maybe it could be
made to work after all. If nothing else, the failing test
case would be useful.

Luke Diamand (2):
  git-p4: add test case for RCS keywords
  git-p4: initial demonstration of possible RCS keyword fixup

 contrib/fast-import/git-p4 |   43 ++++++++++++++++++++++-
 t/t9810-git-p4-rcs.sh      |   81 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+), 2 deletions(-)
 create mode 100755 t/t9810-git-p4-rcs.sh

-- 
1.7.9.rc2.128.gfce41.dirty

[RFC/PATCHv2 1/2] git-p4: add test case for RCS keywords

[Luke Diamand]

RCS keywords cause problems for git-p4 as perforce always
expands them (if +k is set) and so when applying the patch,
git reports that the files have been modified by both sides,
when in fact they haven't.

This adds a failing test case to demonstrate the problem.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9810-git-p4-rcs.sh |   80 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100755 t/t9810-git-p4-rcs.sh

diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
new file mode 100755
index 0000000..c09f83d
--- /dev/null
+++ b/t/t9810-git-p4-rcs.sh
@@ -0,0 +1,80 @@
+#!/bin/sh
+
+test_description='git-p4 rcs keywords'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+create_kw_file() {
+	cat <<\EOF >"$1"
+/* A file
+	Id: $Id$
+	Revision: $Revision$
+	File: $File$
+ */
+int main(int argc, const char **argv) {
+	return 0;
+}
+EOF
+}
+
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "change 1" &&
+		create_kw_file kwfile1.c &&
+		p4 add kwfile1.c &&
+		p4 submit -d "Add rcw kw file" kwfile1.c
+	)
+'
+
+p4_append_to_file() {
+	f=$1 &&
+	p4 edit -t ktext "$f" &&
+	echo "/* $(date) */" >>"$f" &&
+	p4 submit -d "appending a line in p4"
+}
+
+# Create some files with RCS keywords. If they get modified
+# elsewhere then the version number gets bumped which then
+# results in a merge conflict if we touch the RCS kw lines,
+# even though the change itself would otherwise apply cleanly.
+test_expect_failure 'cope with rcs keyword expansion damage' '
+	"$GITP4" clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		(cd ../cli && p4_append_to_file kwfile1.c) &&
+		perl -n -i -e "print unless m/Revision:/" kwfile1.c &&
+		git add kwfile1.c &&
+		git commit -m "Zap an RCS kw line" &&
+		"$GITP4" submit &&
+		"$GITP4" rebase &&
+		git diff p4/master &&
+		"$GITP4" commit &&
+		echo "try modifying in both" &&
+		cd "$cli" &&
+		p4 edit kwfile1.c &&
+		echo "line from p4" >>kwfile1.c &&
+		p4 submit -d "add a line in p4" kwfile1.c &&
+		cd "$git" &&
+		echo "line from git at the top" | cat - kwfile1.c >kwfile1.c.new &&
+		mv kwfile1.c.new kwfile1.c &&
+		git commit -m "Add line in git at the top" kwfile1.c &&
+		"$GITP4" rebase &&
+		"$GITP4" submit
+	) &&
+	rm -rf "$git" && mkdir "$git"
+'
+
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.9.rc2.128.gfce41.dirty

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

[Luke Diamand]

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.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 contrib/fast-import/git-p4 |   43 +++++++++++++++++++++++++++++++++++++++++--
 t/t9810-git-p4-rcs.sh      |    1 +
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index a78d9c5..205fefd 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -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)
+            outFile.write(line)
+        inFile.close()
+        outFile.close()
+        # Forcibly overwrite the original file
+        system("cat %s" % outFileName)
+        system(["mv", "-f", outFileName, file])
+        print "Patched up RCS keywords in %s" % file
+
     def p4UserForCommit(self,id):
         # Return the tuple (perforce user,git email) for a given git commit id
         self.getUserMapFromPerforceServer()
@@ -918,6 +935,7 @@ class P4Submit(Command, P4UserMap):
         filesToDelete = set()
         editedFiles = set()
         filesToChangeExecBit = {}
+
         for line in diff:
             diff = parseDiffTreeEntry(line)
             modifier = diff['status']
@@ -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
+            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)
                 contents = [ text ]
             elif "k" in type_mods:
                 text = ''.join(contents)
-                text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', text)
+                text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$', r'$\1$', text)
                 contents = [ text ]
 
         self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index c09f83d..efe172c 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -49,6 +49,7 @@ test_expect_failure 'cope with rcs keyword expansion damage' '
 	(
 		cd "$git" &&
 		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.attemptRCSCleanup true &&
 		(cd ../cli && p4_append_to_file kwfile1.c) &&
 		perl -n -i -e "print unless m/Revision:/" kwfile1.c &&
 		git add kwfile1.c &&
-- 
1.7.9.rc2.128.gfce41.dirty

Re: [RFC/PATCHv2 0/2] git-p4: possible RCS keyword fixes

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> I said before I wouldn't try to fix this, but maybe it could be
> made to work after all. If nothing else, the failing test
> case would be useful.

Will queue in 'pu' so that git-p4 folks can more easily try it out.  The
variable needs to be added to Documentation/git-p4.txt after people are
satisfied with the resulting code.

Thanks.

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

[Pete Wyckoff]

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

[PATCH] git-p4: more RCS tests

[Pete Wyckoff]

Add a few smaller test cases.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9810-git-p4-rcs.sh |  201 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 196 insertions(+), 5 deletions(-)

diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index a235913..013a951 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -8,6 +8,198 @@ test_expect_success 'start p4d' '
 	start_p4d
 '
 
+#
+# Make one file with keyword lines at the top, and
+# enough plain text to be able to test modifications
+# far away from the keywords.
+#
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		cat <<-\EOF >filek &&
+		$Id$
+		/* $Revision$ */
+		# $Change$
+		line4
+		line5
+		line6
+		line7
+		line8
+		EOF
+		cp filek fileko &&
+		sed -i "s/Revision/Revision: do not scrub me/" fileko
+		cp fileko file_text &&
+		sed -i "s/Id/Id: do not scrub me/" file_text
+		p4 add -t text+k filek &&
+		p4 submit -d "filek" &&
+		p4 add -t text+ko fileko &&
+		p4 submit -d "fileko" &&
+		p4 add -t text file_text &&
+		p4 submit -d "file_text"
+	)
+'
+
+#
+# Generate these in a function to make it easy to use single quote marks.
+#
+write_scrub_scripts() {
+	cat >"$TRASH_DIRECTORY/scrub_k.py" <<-\EOF &&
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', sys.stdin.read()))
+	EOF
+	cat >"$TRASH_DIRECTORY/scrub_ko.py" <<-\EOF
+	import re, sys
+	sys.stdout.write(re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', sys.stdin.read()))
+	EOF
+}
+
+test_expect_success 'scrub scripts' '
+	write_scrub_scripts
+'
+
+#
+# Compare $cli/file to its scrubbed version, should be different.
+# Compare scrubbed $cli/file to $git/file, should be same.
+#
+scrub_k_check() {
+	file=$1 &&
+	scrub="$TRASH_DIRECTORY/$file" &&
+	"$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_k.py" <"$git/$file" >"$scrub" &&
+	! test_cmp "$cli/$file" "$scrub" &&
+	test_cmp "$git/$file" "$scrub" &&
+	rm "$scrub"
+}
+scrub_ko_check() {
+	file=$1 &&
+	scrub="$TRASH_DIRECTORY/$file" &&
+	"$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_ko.py" <"$git/$file" >"$scrub" &&
+	! test_cmp "$cli/$file" "$scrub" &&
+	test_cmp "$git/$file" "$scrub" &&
+	rm "$scrub"
+}
+
+#
+# Modify far away from keywords.  If no RCS lines show up
+# in the diff, there is no conflict.
+#
+test_expect_success 'edit far away from RCS lines' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		sed -i "s/^line7/line7 edit/" filek &&
+		git commit -m "filek line7 edit" filek &&
+		"$GITP4" submit &&
+		scrub_k_check filek
+	)
+'
+
+#
+# Modify near the keywords.  This will require RCS scrubbing.
+#
+test_expect_success 'edit near RCS lines' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.attemptRCSCleanup true &&
+		sed -i "s/^line4/line4 edit/" filek &&
+		git commit -m "filek line4 edit" filek &&
+		"$GITP4" submit &&
+		scrub_k_check filek
+	)
+'
+
+#
+# Modify the keywords themselves.  This also will require RCS scrubbing.
+#
+test_expect_success 'edit keyword lines' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.attemptRCSCleanup true &&
+		sed -i "/Revision/d" filek &&
+		git commit -m "filek remove Revision line" filek &&
+		"$GITP4" submit &&
+		scrub_k_check filek
+	)
+'
+
+#
+# Scrubbing text+ko files should not alter all keywords, just Id, Header.
+#
+test_expect_failure 'scrub ko files differently' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.attemptRCSCleanup true &&
+		sed -i "s/^line4/line4 edit/" fileko &&
+		git commit -m "fileko line4 edit" fileko &&
+		"$GITP4" submit &&
+		scrub_ko_check fileko &&
+		! scrub_k_check fileko
+	)
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure' '
+	(
+		cd "$cli" &&
+		p4 revert ...
+	)
+'
+
+#
+# Do not scrub anything but +k or +ko files.  Sneak a change into
+# the cli file so that submit will get a conflict.  Make sure that
+# scrubbing doesn't make a mess of things.
+#
+# Assumes that git-p4 exits leaving the p4 file open, with the
+# conflict-generating patch unapplied.
+#
+# This might happen only if the git repo is behind the p4 repo at
+# submit time, and there is a conflict.
+#
+test_expect_failure 'do not scrub plain text' '
+	test_when_finished cleanup_git &&
+	"$GITP4" clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.attemptRCSCleanup true &&
+		sed -i "s/^line4/line4 edit/" file_text &&
+		git commit -m "file_text line4 edit" file_text &&
+		(
+			cd "$cli" &&
+			p4 open file_text &&
+			sed -i "s/^line5/line5 p4 edit/" file_text &&
+			p4 submit -d "file5 p4 edit"
+		) &&
+		! "$GITP4" submit &&
+		(
+			# exepct something like:
+			#    file_text - file(s) not opened on this client
+			# but not copious diff output
+			cd "$cli" &&
+			p4 diff file_text >wc &&
+			test_line_count = 1 wc
+		)
+	)
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure 2' '
+	(
+		cd "$cli" &&
+		p4 revert ...
+	)
+'
 create_kw_file () {
 	cat <<\EOF >"$1"
 /* A file
@@ -21,12 +213,12 @@ int main(int argc, const char **argv) {
 EOF
 }
 
-test_expect_success 'init depot' '
+test_expect_success 'add kwfile' '
 	(
 		cd "$cli" &&
 		echo file1 >file1 &&
 		p4 add file1 &&
-		p4 submit -d "change 1" &&
+		p4 submit -d "file1" &&
 		create_kw_file kwfile1.c &&
 		p4 add kwfile1.c &&
 		p4 submit -d "Add rcw kw file" kwfile1.c
@@ -45,6 +237,7 @@ p4_append_to_file () {
 # results in a merge conflict if we touch the RCS kw lines,
 # even though the change itself would otherwise apply cleanly.
 test_expect_failure 'cope with rcs keyword expansion damage' '
+	test_when_finished cleanup_git &&
 	"$GITP4" clone --dest="$git" //depot &&
 	(
 		cd "$git" &&
@@ -69,11 +262,9 @@ test_expect_failure 'cope with rcs keyword expansion damage' '
 		git commit -m "Add line in git at the top" kwfile1.c &&
 		"$GITP4" rebase &&
 		"$GITP4" submit
-	) &&
-	rm -rf "$git" && mkdir "$git"
+	)
 '
 
-
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.7.9.213.g57e99

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

[Luke Diamand]

On 11/02/12 23:42, Pete Wyckoff wrote:
>
> 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.

Thanks - I'm glad you like it! I'll rework it with your code and resubmit.


>
> Some comments in this code below:
>
>
> 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.

I guess scrubbing every file could get quite slow. We have people 
checking in hundreds of megabytes of C test vectors....

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

Good point - I'll put the ':' back.

>
> Impressed.

Thanks!

Updated patch series to follow.


Luke