threads / rfc / 28880

RFC patch, 4 partsgit-p4: small fixes to branches and labels; tests

Subject: [RFC/PATCH 0/4] git-p4: small fixes to branches and labels; tests

## tl;dr

6 messages between Nov 7, 2011 and Nov 7, 2011. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Luke Diamand· Nov 7, 2011, 21:36 UTC · lore

This is a small set of patches to git-p4 to fix a couple of issues with branches and labels.

Firstly, I've added the fixes needed so that branches and labels can contain shell metacharacters (missed from the previous series). Added a test case for this.

In adding the test case for labels I also found a few other small bugs in the label handling:

 - labels missing a description or "EOT" in their text cause problems;
 - labels without an owner cause problems.

I also noticed, but did not fix, that you can't have more than one label per commit (the others are silently dropped) and the documentation for branch import could be improved.

Luke Diamand (4):
  git-p4: handle p4 branches and labels containing shell chars
  git-p4: cope with labels with empty descriptions
  git-p4: importing labels should cope with missing owner
  git-p4: add test for p4 labels
 contrib/fast-import/git-p4 |   61 ++++++++++++++++++++-----------------
 t/t9801-git-p4-branch.sh   |   48 +++++++++++++++++++++++++++++
 t/t9804-git-p4-label.sh    |   73 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+), 28 deletions(-)
 create mode 100755 t/t9804-git-p4-label.sh
-- 
1.7.7.295.g34dd4
Luke Diamand· Nov 7, 2011, 21:36 UTC · re: Luke Diamand · lore

[PATCH 1/4] git-p4: handle p4 branches and labels containing shell chars

Don't use shell expansion when detecting branches, as it will fail if the branch name contains a shell metachar. Similarly for labels.

Add additional test for branches with shell metachars.
---
 contrib/fast-import/git-p4 |    8 +++---
 t/t9801-git-p4-branch.sh   |   48 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 4 deletions(-)
Show changes to 2 files +52 −4

contrib/fast-import/git-p4, t/t9801-git-p4-branch.sh

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index b975d67..bcac6ec 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -793,7 +793,7 @@ class P4Submit(Command, P4UserMap):
     def canChangeChangelists(self):
         # check to see if we have p4 admin or super-user permissions, either of
         # which are required to modify changelists.
-        results = p4CmdList("protects %s" % self.depotPath)
+        results = p4CmdList(["protects", self.depotPath])
         for r in results:
             if r.has_key('perm'):
                 if r['perm'] == 'admin':
@@ -1528,7 +1528,7 @@ class P4Sync(Command, P4UserMap):
     def getLabels(self):
         self.labels = {}
 
-        l = p4CmdList("labels %s..." % ' '.join (self.depotPaths))
+        l = p4CmdList(["labels", "%s..." % ' '.join (self.depotPaths)])
         if len(l) > 0 and not self.silent:
             print "Finding files belonging to labels in %s" % `self.depotPaths`
 
@@ -1570,7 +1570,7 @@ class P4Sync(Command, P4UserMap):
             command = "branches"
 
         for info in p4CmdList(command):
-            details = p4Cmd("branch -o %s" % info["branch"])
+            details = p4Cmd(["branch", "-o", info["branch"]])
             viewIdx = 0
             while details.has_key("View%s" % viewIdx):
                 paths = details["View%s" % viewIdx].split(" ")
@@ -1708,7 +1708,7 @@ class P4Sync(Command, P4UserMap):
         sourceRef = self.gitRefForBranch(sourceBranch)
         #print "source " + sourceBranch
 
-        branchParentChange = int(p4Cmd("changes -m 1 %s...@1,%s" % (sourceDepotPath, firstChange))["change"])
+        branchParentChange = int(p4Cmd(["changes", "-m", "1", "%s...@1,%s" % (sourceDepotPath, firstChange)])["change"])
         #print "branch parent: %s" % branchParentChange
         gitParent = self.gitCommitByP4Change(sourceRef, branchParentChange)
         if len(gitParent) > 0:
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index a25f18d..bd08dff 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -226,6 +226,54 @@ test_expect_success 'git-p4 clone simple branches' '
 	)
 '
 
+# Create a branch with a shell metachar in its name
+#
+# 1. //depot/main
+# 2. //depot/branch$3
+
+test_expect_success 'branch with shell char' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$cli" &&
+
+		mkdir -p main &&
+
+		echo f1 >main/f1 &&
+		p4 add main/f1 &&
+		p4 submit -d "main/f1" &&
+
+		p4 integrate //depot/main/... //depot/branch\$3/... &&
+		p4 submit -d "integrate main to branch\$3" &&
+
+		echo f1 >branch\$3/shell_char_branch_file &&
+		p4 add branch\$3/shell_char_branch_file &&
+		p4 submit -d "branch\$3/shell_char_branch_file" &&
+
+		p4 branch -i <<-EOF &&
+		Branch: branch\$3
+		View: //depot/main/... //depot/branch\$3/...
+		EOF
+
+		p4 edit main/f1 &&
+		echo "a change" >> main/f1 &&
+		p4 submit -d "a change" main/f1 &&
+
+		p4 integrate -b branch\$3 &&
+		p4 resolve -am branch\$3/... &&
+		p4 submit -d "integrate main to branch\$3" &&
+
+		cd "$git" &&
+
+		git config git-p4.branchList main:branch\$3 &&
+		"$GITP4" clone --dest=. --detect-branches //depot@all &&
+		git log --all --graph --decorate --stat &&
+		git reset --hard p4/depot/branch\$3 &&
+		test -f shell_char_branch_file &&
+		test -f f1
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '

-- 
1.7.7.295.g34dd4
Luke Diamand· Nov 7, 2011, 21:36 UTC · re: Luke Diamand · lore

[PATCH 2/4] git-p4: cope with labels with empty descriptions

Use an explicit length for the data in a label, rather
than EOT, so that labels with empty descriptions are
passed through correctly.
---
 contrib/fast-import/git-p4 |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
Show changes to contrib/fast-import/git-p4 +5 −3
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index bcac6ec..02f0f54 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1511,9 +1511,11 @@ class P4Sync(Command, P4UserMap):
                     else:
                         tagger = "%s <a@b> %s %s" % (owner, epoch, self.tz)
                     self.gitStream.write("tagger %s\n" % tagger)
-                    self.gitStream.write("data <<EOT\n")
-                    self.gitStream.write(labelDetails["Description"])
-                    self.gitStream.write("EOT\n\n")
+
+                    description = labelDetails["Description"]
+                    self.gitStream.write("data %d\n" % len(description))
+                    self.gitStream.write(description)
+                    self.gitStream.write("\n")
 
                 else:
                     if not self.silent:

-- 
1.7.7.295.g34dd4
Luke Diamand· Nov 7, 2011, 21:36 UTC · re: Luke Diamand · lore

[PATCH 3/4] git-p4: importing labels should cope with missing owner

In p4, the Owner field is optional. If it is missing,
construct something sensible rather than crashing.
---
 contrib/fast-import/git-p4 |   45 +++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 21 deletions(-)
Show changes to contrib/fast-import/git-p4 +24 −21
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 02f0f54..d97f927 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -553,6 +553,27 @@ class Command:
     def __init__(self):
         self.usage = "usage: %prog [options]"
         self.needsGit = True
+        self.myP4UserId = None
+
+    def p4UserId(self):
+        if self.myP4UserId:
+            return self.myP4UserId
+
+        results = p4CmdList("user -o")
+        for r in results:
+            if r.has_key('User'):
+                self.myP4UserId = r['User']
+                return r['User']
+        die("Could not find your p4 user id")
+
+    def p4UserIsMe(self, p4User):
+        # return True if the given p4 user is actually me
+        me = self.p4UserId()
+        if not p4User or p4User != me:
+            return False
+        else:
+            return True
+
 
 class P4UserMap:
     def __init__(self):
@@ -694,7 +715,6 @@ class P4Submit(Command, P4UserMap):
         self.verbose = False
         self.preserveUser = gitConfig("git-p4.preserveUser").lower() == "true"
         self.isWindows = (platform.system() == "Windows")
-        self.myP4UserId = None
 
     def check(self):
         if len(p4CmdList("opened ...")) > 0:
@@ -802,25 +822,6 @@ class P4Submit(Command, P4UserMap):
                     return 1
         return 0
 
-    def p4UserId(self):
-        if self.myP4UserId:
-            return self.myP4UserId
-
-        results = p4CmdList("user -o")
-        for r in results:
-            if r.has_key('User'):
-                self.myP4UserId = r['User']
-                return r['User']
-        die("Could not find your p4 user id")
-
-    def p4UserIsMe(self, p4User):
-        # return True if the given p4 user is actually me
-        me = self.p4UserId()
-        if not p4User or p4User != me:
-            return False
-        else:
-            return True
-
     def prepareSubmitTemplate(self):
         # remove lines in the Files section that show changes to files outside the depot path we're committing into
         template = ""
@@ -1506,7 +1507,9 @@ class P4Sync(Command, P4UserMap):
 
                     owner = labelDetails["Owner"]
                     tagger = ""
-                    if author in self.users:
+                    if not owner:
+                        tagger = "%s <a@b> %s %s" % (self.p4UserId(), epoch, self.tz)
+                    elif author in self.users:
                         tagger = "%s %s %s" % (self.users[owner], epoch, self.tz)
                     else:
                         tagger = "%s <a@b> %s %s" % (owner, epoch, self.tz)

-- 
1.7.7.295.g34dd4
Pete Wyckoff· Nov 7, 2011, 22:34 UTC · re: Luke Diamand · lore

Re: [PATCH 3/4] git-p4: importing labels should cope with missing owner

luke@diamand.org wrote on Mon, 07 Nov 2011 21:36 +0000:
> In p4, the Owner field is optional. If it is missing,
> construct something sensible rather than crashing.
Nice.  One question:
Show 9 quoted lines
>                      owner = labelDetails["Owner"]
>                      tagger = ""
> -                    if author in self.users:
> +                    if not owner:
> +                        tagger = "%s <a@b> %s %s" % (self.p4UserId(), epoch, self.tz)
> +                    elif author in self.users:
>                          tagger = "%s %s %s" % (self.users[owner], epoch, self.tz)
>                      else:
>                          tagger = "%s <a@b> %s %s" % (owner, epoch, self.tz)

We trust p4 always returns a key "Owner", but it could be an empty string? I'm okay with that, if that's how it works.

Should be "author in self.users"? I'm looking at the creation of the committer string above, and guessing that we try to use owner, and if that doesn't exist, try to use author just like as above, so something like:

    if owner:
	if owner in self.users:
	    email = self.users[owner]
	else:
	    email = "%s <a@b>" % owner
    else:
	if author in self.users:
	    email = self.users[author]
	else:
	    email = "%s <a@b>" % author
I could misunderstand this.

Maybe that lookup in self.users[] and <a@b> default wants to have a function since it will be repeated three times now.

Ack to the whole series.
		-- Pete
Luke Diamand· Nov 7, 2011, 21:36 UTC · re: Luke Diamand · lore

[PATCH 4/4] git-p4: add test for p4 labels

Add basic test of p4 label import. Checks label import and
import with shell metachars; labels with different length
descriptions.
---
 t/t9804-git-p4-label.sh |   73 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 73 insertions(+), 0 deletions(-)
 create mode 100755 t/t9804-git-p4-label.sh
Show changes to t/t9804-git-p4-label.sh +73 −0
diff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh
new file mode 100755
index 0000000..eba3521
--- /dev/null
+++ b/t/t9804-git-p4-label.sh
@@ -0,0 +1,73 @@
+test_description='git-p4 p4 label tests'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+# Basic p4 label tests.
+#
+# Note: can't have more than one label per commit - others
+# are silently discarded.
+#
+test_expect_success 'basic p4 labels' '
+	test_when_finished cleanup_git &&
+	(
+		cd "$cli" &&
+		mkdir -p main &&
+
+		echo f1 >main/f1 &&
+		p4 add main/f1 &&
+		p4 submit -d "main/f1" &&
+
+		echo f2 >main/f2 &&
+		p4 add main/f2 &&
+		p4 submit -d "main/f2" &&
+
+		echo f3 >main/file_with_\$metachar &&
+		p4 add main/file_with_\$metachar &&
+		p4 submit -d "file with metachar" &&
+
+		p4 tag -l tag_f1_only main/f1 &&
+		p4 tag -l tag_with\$_shell_char main/... &&
+
+		echo f4 >main/f4 &&
+		p4 add main/f4 &&
+		p4 submit -d "main/f4" &&
+
+		p4 label -i <<-EOF &&
+		Label: long_label
+		Description:
+		   A Label first line
+		   A Label second line
+		View:	//depot/...
+		EOF
+
+		p4 tag -l long_label ... &&
+
+		p4 labels ... &&
+
+		cd "$git" &&
+		pwd &&
+		"$GITP4" clone --dest=. --detect-labels //depot@all &&
+
+		git tag &&
+		git tag >taglist &&
+		test_line_count = 3 taglist &&
+
+		cd main &&
+		git checkout tag_tag_f1_only &&
+		! test -f f2 &&
+		git checkout tag_tag_with\$_shell_char &&
+		test -f f1 && test -f f2 && test -f file_with_\$metachar &&
+
+		git show tag_long_label | grep -q "A Label second line"
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done

-- 
1.7.7.295.g34dd4

← back to recent threads