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

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

[Luke Diamand]

This contains a possible way to fixup RCS keyword woes in git-p4.

The first patch adds a test case to demonstrate the problem. It would
be useful to get this into git proper.

The second patch shows a possible way to solve the problem, by
patching up the RCS keywords in the p4 checked-out tree. This
patch does not cover all cases and is poorly tested. I'd like to
see if this seems like a plausible approach.

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      |   82 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 2 deletions(-)
 create mode 100755 t/t9810-git-p4-rcs.sh

-- 
1.7.9.rc2.128.gfce41.dirty

[PATCHv4 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 |   81 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 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..bad6272
--- /dev/null
+++ b/t/t9810-git-p4-rcs.sh
@@ -0,0 +1,81 @@
+#!/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" &&
+ cat $f
+}
+
+# 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 &&
+
+ cd "$TRASH_DIRECTORY" &&
+ rm -rf "$git" && mkdir "$git"
+'
+
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.7.9.rc2.128.gfce41.dirty

[PATCHv4 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 bad6272..dc38dcc 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' '
  "$GITP4" clone --dest="$git" //depot &&
  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: [PATCHv4 1/2] git-p4: add test case for RCS keywords

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> +create_kw_file() {
> + cat <<'EOF' > $1

Please start this shell function like this instead:

        create_kw_file () {
                cat <<\EOF >"$1"

The first line is merely cosmetic but matches the convention used in our
shell scripts better.

As to the second line:

 * Quoting "$1" should not be strictly necessary according to POSIX rules,
   but we saw reports that some shells "helpfully" give unnecessary
   warnings when $1 (or whatever variable that holds the name of the file
   the output is redirected into) contains $IFS characters.

 * An indent in our shell scripts is a HT, not a SP (or four spaces or
   whatever).

 * And our convention is not to have an extra SP after redirection '>' (or
   '<') operator.

> +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" &&
> + cat $f
> +}

Surround all occurrences of '$f' with double-quotes, e.g.

	cat "$f"

to avoid unexpected breakage when later somebody tries to use a file whose
name happens to contain a space. The first assignment "f=$1" can stay as-is.

> +
> +# 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" &&

Please do not cd around in test script outside a subshell.  Otherwise, any
failure in a subsequent command in this && chain before you reach the next
"cd" will leave you in an unexpected directory and can break later tests.

The worst example would be

	test_expect_something 'one' '
		mkdir -p git/play/pen &&
                cd git/play/pen &&
                do something &&
        	cd ../../.. &&
                do something that potentially can fail &&
                cd git/play/pen &&
                do something
	'
	# blindly assuming the above succeeded up to the last 'cd git'
        test_expect_something 'two' '
		cd ../../.. &&
        	rm -fr git &&
                do something else
	'

If the first test fails in the middle before it brings you back down to
'git/play/pen' with the last 'cd git/playpen', then the next test moves
you up three levels and you will be running "rm -fr" on a wrong 'git'!