threads / rfc / 29579

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

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

## tl;dr

4 messages between Feb 9, 2012 and Feb 9, 2012.

replies: 3people: 2as markdown or json

Luke Diamand· Feb 9, 2012, 11:03 UTC · lore
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
Luke Diamand· Feb 9, 2012, 11:03 UTC · re: Luke Diamand · lore

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

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
Junio C Hamano· Feb 9, 2012, 22:55 UTC · re: Luke Diamand · lore

Re: [PATCHv4 1/2] git-p4: add test case for RCS keywords

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.
Show 19 quoted lines
> +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.

Show 8 quoted lines
> +
> +# 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'!

Luke Diamand· Feb 9, 2012, 11:03 UTC · re: Luke Diamand · lore

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

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

← back to recent threads