threads / discuss / 29378

[PATCHv3 0/4] git-p4: small fixes to branches and labels

Subject: [PATCHv3 0/4] git-p4: small fixes to branches and labels

## tl;dr

7 messages between Jan 16, 2012 and Jan 19, 2012.

replies: 6people: 2as markdown or json

Luke Diamand· Jan 16, 2012, 23:14 UTC · lore

This is the third version of some small fixes to git-p4 branch and label handling.

It was suggested for the earlier version that the author handling could be simplified to use the 'author' variable. The code can be simplified, but the author is the wrong value to use - it is just the author of the commit, not the tag. Use the creator of the label, or, if that does not exist ("p4 tag ..."), the p4 user.

This change does not fix the other problems with git-p4 labels:
- two p4 labels on the same changelist will fall over
- labels must match exactly the list of files imported
- you can't import a label without a p4 commit
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        |   79 ++++++++++++++++++++----------------
 t/t9803-git-p4-shell-metachars.sh |   48 ++++++++++++++++++++++
 t/t9804-git-p4-label.sh           |   73 ++++++++++++++++++++++++++++++++++
 3 files changed, 165 insertions(+), 35 deletions(-)
 create mode 100755 t/t9804-git-p4-label.sh
-- 
1.7.8.rc1.209.geac91.dirty
Luke Diamand· Jan 16, 2012, 23:14 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.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
 contrib/fast-import/git-p4        |    8 +++---
 t/t9803-git-p4-shell-metachars.sh |   48 +++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 3e1aa27..822e6f1 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -799,7 +799,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':
@@ -1758,7 +1758,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`
 
@@ -1800,7 +1800,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(" ")
@@ -1938,7 +1938,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/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
index db04375..db67020 100755
--- a/t/t9803-git-p4-shell-metachars.sh
+++ b/t/t9803-git-p4-shell-metachars.sh
@@ -57,6 +57,54 @@ test_expect_success 'deleting with shell metachars' '
 	)
 '
 
+# 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.8.rc1.209.geac91.dirty
Pete Wyckoff· Jan 17, 2012, 22:39 UTC · re: Luke Diamand · lore

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

luke@diamand.org wrote on Mon, 16 Jan 2012 23:14 +0000:
Show 5 quoted lines
> 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.

Nice. There will be a fixup on a command in Vitor's series, depending on which goes first. He'll have a couple of un-listified read_pipe{,_lines} that we should treat similarly.

Show 8 quoted lines
> @@ -1758,7 +1758,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`

I suspect the command "p4" "labels" "//depot/foo/... //depot/bar/..." might confuse p4, but haven't tested. Maybe tuck each one in its own argument?

    ["labels"] + ["%s..." % p for p in self.depotPaths]

What happened to your failing test? It's fun to keep the broken ones around to inspire others to fix them.

		-- Pete
Luke Diamand· Jan 19, 2012, 09:12 UTC · re: Pete Wyckoff · lore

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

On 17/01/12 22:39, Pete Wyckoff wrote:
Show 25 quoted lines
> luke@diamand.org wrote on Mon, 16 Jan 2012 23:14 +0000:
>> 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.
>
> Nice.  There will be a fixup on a command in Vitor's series,
> depending on which goes first.  He'll have a couple of
> un-listified read_pipe{,_lines} that we should treat similarly.
>
>> @@ -1758,7 +1758,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`
>
> I suspect the command "p4" "labels" "//depot/foo/...  //depot/bar/..."
> might confuse p4, but haven't tested.  Maybe tuck each one in its
> own argument?
>
>      ["labels"] + ["%s..." % p for p in self.depotPaths]

Yes, you're right. I'll resubmit. I suspect the previous code was actually broken as well as you end up with the "..." only on the last depot.

>
> What happened to your failing test?  It's fun to keep the broken
> ones around to inspire others to fix them.

I'll send that one through as well. I actually had a fix, but it needs to be reworked now. I'm reluctant to try to do much more on this though unless someone can tell me how the --detect-labels code can ever really work.

i.e. if you do:
  p4 submit
  git-p4 rebase --detect-labels
  p4 tag LABEL_A
  git-p4 rebase --detect-labels
At this point, LABEL_A won't show up in git until the _next_ p4 commit.
It's slowly working it's way towards the top of my todo list though!

Regards! Luke

Luke Diamand· Jan 16, 2012, 23:14 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.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 contrib/fast-import/git-p4 |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 822e6f1..f7707f2 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1741,9 +1741,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.8.rc1.209.geac91.dirty
Luke Diamand· Jan 16, 2012, 23:14 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.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 contrib/fast-import/git-p4 |   63 ++++++++++++++++++++++++-------------------
 1 files changed, 35 insertions(+), 28 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index f7707f2..efb2dad 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -563,6 +563,26 @@ class Command:
 class P4UserMap:
     def __init__(self):
         self.userMapFromPerforceServer = False
+        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
 
     def getUserCacheFilename(self):
         home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
@@ -700,7 +720,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:
@@ -808,25 +827,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 = ""
@@ -1664,6 +1664,12 @@ class P4Sync(Command, P4UserMap):
             if self.stream_file.has_key('depotFile'):
                 self.streamOneP4File(self.stream_file, self.stream_contents)
 
+    def make_email(self, userid):
+        if userid in self.users:
+            return self.users[userid]
+        else:
+            return "%s <a@b>" % userid
+
     def commit(self, details, files, branch, branchPrefixes, parent = ""):
         epoch = details["time"]
         author = details["user"]
@@ -1687,10 +1693,7 @@ class P4Sync(Command, P4UserMap):
         committer = ""
         if author not in self.users:
             self.getUserMapFromPerforceServer()
-        if author in self.users:
-            committer = "%s %s %s" % (self.users[author], epoch, self.tz)
-        else:
-            committer = "%s <a@b> %s %s" % (author, epoch, self.tz)
+        committer = "%s %s %s" % (self.make_email(author), epoch, self.tz)
 
         self.gitStream.write("committer %s\n" % committer)
 
@@ -1735,11 +1738,15 @@ class P4Sync(Command, P4UserMap):
                     self.gitStream.write("from %s\n" % branch)
 
                     owner = labelDetails["Owner"]
-                    tagger = ""
-                    if author in self.users:
-                        tagger = "%s %s %s" % (self.users[owner], epoch, self.tz)
+
+                    # Try to use the owner of the p4 label, or failing that,
+                    # the current p4 user id.
+                    if owner:
+                        email = self.make_email(owner)
                     else:
-                        tagger = "%s <a@b> %s %s" % (owner, epoch, self.tz)
+                        email = self.make_email(self.p4UserId())
+                    tagger = "%s %s %s" % (email, epoch, self.tz)
+
                     self.gitStream.write("tagger %s\n" % tagger)
 
                     description = labelDetails["Description"]
-- 
1.7.8.rc1.209.geac91.dirty
Luke Diamand· Jan 16, 2012, 23:14 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.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9804-git-p4-label.sh |   73 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 73 insertions(+), 0 deletions(-)
 create mode 100755 t/t9804-git-p4-label.sh
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.8.rc1.209.geac91.dirty

← back to recent threads