{"thread":{"id":"29414","subject":"[PATCH v2 1/3] git-p4: Add checkpoint() task","startedAt":"2012-01-21T00:21:29Z","lastAt":"2012-01-25T12:34:25Z","messageCount":15,"participants":["Vitor Antunes","Junio C Hamano","Luke Diamand","Pete Wyckoff"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"182888","messageId":"1327105292-30092-1-git-send-email-vitor.hda@gmail.com","threadId":"29414","inReplyTo":null,"subject":"[PATCH v2 0/3] git-p4: Search for parent commit on branch creation","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-21T00:21:29Z","receivedAt":"2012-01-21T00:21:29Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"The new version of these patches includes the improvement suggestions\nfrom Pete Wyckoff:\n\n - Use os.path.join().\n - Use git update-ref -d to remove refs.\n - Read $GIT_DIR environment variable.\n - Optimize comparison cycle to avoid excessive use of checkpoint\n   command.\n\nVitor Antunes (3):\n  git-p4: Add checkpoint() task\n  git-p4: Search for parent commit on branch creation\n  git-p4: Add test case for complex branch import\n\n contrib/fast-import/git-p4 |   38 ++++++++++++++++++++-\n t/t9801-git-p4-branch.sh   |   80 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 117 insertions(+), 1 deletions(-)\n\n-- \n1.7.7.rc2.14.g5e044.dirty\n"},{"id":"182887","messageId":"1327105292-30092-2-git-send-email-vitor.hda@gmail.com","threadId":"29414","inReplyTo":"1327105292-30092-1-git-send-email-vitor.hda@gmail.com","subject":"[PATCH v2 1/3] git-p4: Add checkpoint() task","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-21T00:21:30Z","receivedAt":"2012-01-21T00:21:30Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>\n---\n contrib/fast-import/git-p4 |    8 ++++++++\n 1 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 3e1aa27..417d119 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -1450,6 +1450,14 @@ class P4Sync(Command, P4UserMap):\n                    .replace(\"%25\", \"%\")\n         return path\n \n+    # Force a checkpoint in fast-import and wait for it to finish\n+    def checkpoint(self):\n+        self.gitStream.write(\"checkpoint\\n\\n\")\n+        self.gitStream.write(\"progress checkpoint\\n\\n\")\n+        out = self.gitOutput.readline()\n+        if self.verbose:\n+            print \"checkpoint finished: \" + out\n+\n     def extractFilesFromCommit(self, commit):\n         self.cloneExclude = [re.sub(r\"\\.\\.\\.$\", \"\", path)\n                              for path in self.cloneExclude]\n-- \n1.7.7.rc2.14.g5e044.dirty\n"},{"id":"182890","messageId":"1327105292-30092-3-git-send-email-vitor.hda@gmail.com","threadId":"29414","inReplyTo":"1327105292-30092-1-git-send-email-vitor.hda@gmail.com","subject":"[PATCH v2 2/3] git-p4: Search for parent commit on branch creation","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-21T00:21:31Z","receivedAt":"2012-01-21T00:21:31Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"To find out which is its parent the commit of the new branch is applied\nsequentially to each blob of the parent branch from the newest to the\noldest. The first blob which results in a zero diff is considered the\nparent commit. If none is found, then the commit is applied to the top\nof the parent branch.\n\nA fast-import \"checkpoint\" call is required for each comparison because\ndiff-tree is only able to work with blobs on disk. But most of these\ncommits will not be part of the final imported tree, making fast-import\nfail. To avoid this, the temporary branches are tracked and then removed\nat the end of the import process.\n\nSigned-off-by: Vitor Antunes <vitor.hda@gmail.com>\n---\n contrib/fast-import/git-p4 |   30 +++++++++++++++++++++++++++++-\n 1 files changed, 29 insertions(+), 1 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 417d119..2e3b741 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -1429,6 +1429,8 @@ class P4Sync(Command, P4UserMap):\n         self.cloneExclude = []\n         self.useClientSpec = False\n         self.clientSpecDirs = None\n+        self.tempBranches = []\n+        self.tempBranchLocation = \"git-p4-tmp\"\n \n         if gitConfig(\"git-p4.syncFromOrigin\") == \"false\":\n             self.syncWithOrigin = False\n@@ -2012,7 +2014,27 @@ class P4Sync(Command, P4UserMap):\n                             parent = self.initialParents[branch]\n                             del self.initialParents[branch]\n \n-                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)\n+                        parentFound = False\n+                        if len(parent) > 0:\n+                            tempBranch = os.path.join(self.tempBranchLocation, \"%d\" % (change))\n+                            if self.verbose:\n+                                print \"Creating temporary branch: \" + tempBranch\n+                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])\n+                            self.tempBranches.append(tempBranch)\n+                            self.checkpoint()\n+                            for blob in read_pipe_lines(\"git rev-list --reverse --no-merges %s\" % parent):\n+                                blob = blob.strip()\n+                                if len( read_pipe(\"git diff-tree %s %s\" % (blob, tempBranch)) ) == 0:\n+                                    parentFound = True\n+                                    if self.verbose:\n+                                        print \"Found parent of %s in commit %s\" % (branch, blob)\n+                                    break\n+                        if parentFound:\n+                            self.commit(description, filesForCommit, branch, [branchPrefix], blob)\n+                        else:\n+                            if self.verbose:\n+                                print \"Parent of %s not found. Committing into head of %s\" % (branch, parent)\n+                            self.commit(description, filesForCommit, branch, [branchPrefix], parent)\n                 else:\n                     files = self.extractFilesFromCommit(description)\n                     self.commit(description, files, self.branch, self.depotPaths,\n@@ -2347,6 +2369,12 @@ class P4Sync(Command, P4UserMap):\n         self.gitOutput.close()\n         self.gitError.close()\n \n+        # Cleanup temporary branches created during import\n+        if self.tempBranches != []:\n+            for branch in self.tempBranches:\n+                read_pipe(\"git update-ref -d %s\" % branch)\n+            os.rmdir(os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), self.tempBranchLocation))\n+\n         return True\n \n class P4Rebase(Command):\n-- \n1.7.7.rc2.14.g5e044.dirty\n"},{"id":"182889","messageId":"1327105292-30092-4-git-send-email-vitor.hda@gmail.com","threadId":"29414","inReplyTo":"1327105292-30092-1-git-send-email-vitor.hda@gmail.com","subject":"[PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-21T00:21:32Z","receivedAt":"2012-01-21T00:21:32Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"Check if branches created from old changelists are correctly imported.\n\nSigned-off-by: Vitor Antunes <vitor.hda@gmail.com>\n---\n t/t9801-git-p4-branch.sh |   80 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 80 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh\nindex a25f18d..ed4e48c 100755\n--- a/t/t9801-git-p4-branch.sh\n+++ b/t/t9801-git-p4-branch.sh\n@@ -226,6 +226,86 @@ test_expect_success 'git-p4 clone simple branches' '\n \t)\n '\n \n+# Create a complex branch structure in P4 depot to check if they are correctly\n+# cloned. The branches are created from older changelists to check if git-p4 is\n+# able to correctly detect them.\n+# The final expected structure is:\n+# `branch1\n+# | `- file1\n+# | `- file2 (updated)\n+# | `- file3\n+# `branch2\n+# | `- file1\n+# | `- file2\n+# `branch3\n+# | `- file1\n+# | `- file2 (updated)\n+# | `- file3\n+# `branch4\n+# | `- file1\n+# | `- file2\n+# `branch5\n+#   `- file1\n+#   `- file2\n+#   `- file3\n+test_expect_success 'git-p4 add complex branches' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_create_repo \"$git\" &&\n+\t(\n+\t\tcd \"$cli\" &&\n+\t\tchangelist=$(p4 changes -m1 //depot/... | cut -d\" \" -f2) &&\n+\t\tchangelist=$(($changelist - 5)) &&\n+\t\tp4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&\n+\t\tp4 submit -d \"branch4\" &&\n+\t\tchangelist=$(($changelist + 2)) &&\n+\t\tp4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&\n+\t\tp4 submit -d \"branch5\"\n+\t)\n+'\n+\n+# Configure branches through git-config and clone them. git-p4 will only be able\n+# to clone the original structure if it is able to detect the origin changelist\n+# of each branch.\n+test_expect_success 'git-p4 clone complex branches' '\n+\ttest_when_finished cleanup_git &&\n+\ttest_create_repo \"$git\" &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit config git-p4.branchList branch1:branch2 &&\n+\t\tgit config --add git-p4.branchList branch1:branch3 &&\n+\t\tgit config --add git-p4.branchList branch1:branch4 &&\n+\t\tgit config --add git-p4.branchList branch1:branch5 &&\n+\t\t\"$GITP4\" clone --dest=. --detect-branches //depot@all &&\n+\t\tgit log --all --graph --decorate --stat &&\n+\t\tgit reset --hard p4/depot/branch1 &&\n+\t\ttest_path_is_file file1 &&\n+\t\ttest_path_is_file file2 &&\n+\t\ttest_path_is_file file3 &&\n+\t\tgrep -q update file2 &&\n+\t\tgit reset --hard p4/depot/branch2 &&\n+\t\ttest_path_is_file file1 &&\n+\t\ttest_path_is_file file2 &&\n+\t\ttest_path_is_missing file3 &&\n+\t\t! grep -q update file2 &&\n+\t\tgit reset --hard p4/depot/branch3 &&\n+\t\ttest_path_is_file file1 &&\n+\t\ttest_path_is_file file2 &&\n+\t\ttest_path_is_file file3 &&\n+\t\tgrep -q update file2 &&\n+\t\tgit reset --hard p4/depot/branch4 &&\n+\t\ttest_path_is_file file1 &&\n+\t\ttest_path_is_file file2 &&\n+\t\ttest_path_is_missing file3 &&\n+\t\t! grep -q update file2 &&\n+\t\tgit reset --hard p4/depot/branch5 &&\n+\t\ttest_path_is_file file1 &&\n+\t\ttest_path_is_file file2 &&\n+\t\ttest_path_is_file file3 &&\n+\t\t! grep -q update file2 &&\n+\t\ttest_path_is_missing .git/git-p4-tmp\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n1.7.7.rc2.14.g5e044.dirty\n"},{"id":"182892","messageId":"7vehutd59p.fsf@alter.siamese.dyndns.org","threadId":"29414","inReplyTo":"1327105292-30092-4-git-send-email-vitor.hda@gmail.com","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-21T04:54:42Z","receivedAt":"2012-01-21T04:54:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vitor Antunes <vitor.hda@gmail.com> writes:\n\n> +test_expect_success 'git-p4 add complex branches' '\n> +\ttest_when_finished cleanup_git &&\n> +\ttest_create_repo \"$git\" &&\n> +\t(\n> +\t\tcd \"$cli\" &&\n> +\t\tchangelist=$(p4 changes -m1 //depot/... | cut -d\" \" -f2) &&\n> +\t\tchangelist=$(($changelist - 5)) &&\n> +\t\tp4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&\n> +\t\tp4 submit -d \"branch4\" &&\n> +\t\tchangelist=$(($changelist + 2)) &&\n> +\t\tp4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&\n> +\t\tp4 submit -d \"branch5\"\n\nThat's a strange quoting convention. Why are \"branch4\" and \"branch5\"\nenclosed in double quotes while \"integrate\" and \"submit\" aren't?\n(rhetorical: do not quote these branch names without a good reason).\n\n> +# Configure branches through git-config and clone them. git-p4 will only be able\n> +# to clone the original structure if it is able to detect the origin changelist\n> +# of each branch.\n> +test_expect_success 'git-p4 clone complex branches' '\n> +\ttest_when_finished cleanup_git &&\n> +\ttest_create_repo \"$git\" &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\tgit config git-p4.branchList branch1:branch2 &&\n> +\t\tgit config --add git-p4.branchList branch1:branch3 &&\n> +\t\tgit config --add git-p4.branchList branch1:branch4 &&\n> +\t\tgit config --add git-p4.branchList branch1:branch5 &&\n> +\t\t\"$GITP4\" clone --dest=. --detect-branches //depot@all &&\n> +\t\tgit log --all --graph --decorate --stat &&\n> +\t\tgit reset --hard p4/depot/branch1 &&\n> +\t\ttest_path_is_file file1 &&\n> +\t\ttest_path_is_file file2 &&\n> +\t\ttest_path_is_file file3 &&\n> +\t\tgrep -q update file2 &&\n\nDo you really need to use \"-q\" here?  Wouldn't it help if you wrote it\nwithout it while debugging tests with \"sh ./t9801-*.sh -v\"?\n\nAlso how does this series interact with the series Luke posted earlier on\nbranches and labels?\n\nThanks.\n"},{"id":"182893","messageId":"7v7h0ld58z.fsf@alter.siamese.dyndns.org","threadId":"29414","inReplyTo":"1327105292-30092-3-git-send-email-vitor.hda@gmail.com","subject":"Re: [PATCH v2 2/3] git-p4: Search for parent commit on branch creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-21T04:55:08Z","receivedAt":"2012-01-21T04:55:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vitor Antunes <vitor.hda@gmail.com> writes:\n\n> A fast-import \"checkpoint\" call is required for each comparison because\n> diff-tree is only able to work with blobs on disk. But most of these\n> commits will not be part of the final imported tree, making fast-import\n> fail. To avoid this, the temporary branches are tracked and then removed\n> at the end of the import process.\n>\n> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>\n> ---\n\nIt might make sense to squash 1/3 with this patch; the definition alone\nwithout any actual user would not be very useful and won't help hunting\nfor breakages, if exists, in the future.\n\n> @@ -2012,7 +2014,27 @@ class P4Sync(Command, P4UserMap):\n>                              parent = self.initialParents[branch]\n>                              del self.initialParents[branch]\n>  \n> -                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)\n> +                        parentFound = False\n> +                        if len(parent) > 0:\n> +                            tempBranch = os.path.join(self.tempBranchLocation, \"%d\" % (change))\n> +                            if self.verbose:\n> +                                print \"Creating temporary branch: \" + tempBranch\n> +                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])\n> +                            self.tempBranches.append(tempBranch)\n> +                            self.checkpoint()\n> +                            for blob in read_pipe_lines(\"git rev-list --reverse --no-merges %s\" % parent):\n> +                                blob = blob.strip()\n> +                                if len( read_pipe(\"git diff-tree %s %s\" % (blob, tempBranch)) ) == 0:\n> +                                    parentFound = True\n> +                                    if self.verbose:\n> +                                        print \"Found parent of %s in commit %s\" % (branch, blob)\n\n... also this looks excessively deeply nested, which is a sign that it\nmight be a good idea to refactor this part into a separate helper function\nor something.\n"},{"id":"182896","messageId":"4F1A98A3.2090607@diamand.org","threadId":"29414","inReplyTo":"7vehutd59p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2012-01-21T10:51:15Z","receivedAt":"2012-01-21T10:51:15Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 21/01/12 04:54, Junio C Hamano wrote:\n> Vitor Antunes<vitor.hda@gmail.com>  writes:\n>\n>> +\t\tgrep -q update file2&&\n>\n> Do you really need to use \"-q\" here?  Wouldn't it help if you wrote it\n> without it while debugging tests with \"sh ./t9801-*.sh -v\"?\n>\n> Also how does this series interact with the series Luke posted earlier on\n> branches and labels?\n\nVitor's series applies cleanly to my changes.\n\nHowever, one thing I noticed in reading through is that it will break if \nyou end up importing a P4 branch that has spaces (or other shell chars) \nin its name. A quick test confirms this.\n\n- the code doesn't handle the names properly\n- git and p4 have different ideas about valid branch names\n\nBut before rejecting Vitor's changes because of that it would be worth \nconsidering whether we care (much). My own opinion is that if you have \ndevelopers who are daft enough to put spaces or dollars in their branch \nnames then their project is already doomed anyway....\n\nPerhaps it would be enough just to issue a warning (\"your project is \ndoomed; start working on your CV\") and skip such branch names rather \nthan falling over with inexplicable error messages.\n\n\n>\n> Thanks.\n"},{"id":"182898","messageId":"20120121171130.GA6235@padd.com","threadId":"29414","inReplyTo":"4F1A98A3.2090607@diamand.org","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-01-21T17:11:30Z","receivedAt":"2012-01-21T17:11:30Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"luke@diamand.org wrote on Sat, 21 Jan 2012 10:51 +0000:\n> On 21/01/12 04:54, Junio C Hamano wrote:\n> >Vitor Antunes<vitor.hda@gmail.com>  writes:\n> >\n> >>+\t\tgrep -q update file2&&\n> >\n> >Do you really need to use \"-q\" here?  Wouldn't it help if you wrote it\n> >without it while debugging tests with \"sh ./t9801-*.sh -v\"?\n> >\n> >Also how does this series interact with the series Luke posted earlier on\n> >branches and labels?\n> \n> Vitor's series applies cleanly to my changes.\n> \n> However, one thing I noticed in reading through is that it will\n> break if you end up importing a P4 branch that has spaces (or other\n> shell chars) in its name. A quick test confirms this.\n> \n> - the code doesn't handle the names properly\n> - git and p4 have different ideas about valid branch names\n> \n> But before rejecting Vitor's changes because of that it would be\n> worth considering whether we care (much). My own opinion is that if\n> you have developers who are daft enough to put spaces or dollars in\n> their branch names then their project is already doomed anyway....\n> \n> Perhaps it would be enough just to issue a warning (\"your project is\n> doomed; start working on your CV\") and skip such branch names rather\n> than falling over with inexplicable error messages.\n\nThis doesn't seem like a big deal.  The read_pipe and\nread_pipe_lines calls shoud be list-ified.  That gets rid\nof the problem with shell interactions.\n\nFor git branch name reserved characters, a little function\nto replace the bogus characters with \"_\" would avoid needing\nto go work on the resume.  Anything in bad_ref_char() and\ncheck_refname_component().  I agree this doesn't have to be\nperfect.\n\nThis could be a new patch unrelated to Vitor's series, which\nverifies branch names anywhere a new commit is made.\n\n\t\t-- Pete\n"},{"id":"182961","messageId":"CAOpHH-Xy2DnoyL_sYjz8umbOg3Hd7iirUzUnfLmYdcg7Oiw6fQ@mail.gmail.com","threadId":"29414","inReplyTo":"7v7h0ld58z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] git-p4: Search for parent commit on branch creation","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-23T13:49:12Z","receivedAt":"2012-01-23T13:49:12Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"Hi Junio,\n\nOn Sat, Jan 21, 2012 at 4:55 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Vitor Antunes <vitor.hda@gmail.com> writes:\n>\n>> A fast-import \"checkpoint\" call is required for each comparison because\n>> diff-tree is only able to work with blobs on disk. But most of these\n>> commits will not be part of the final imported tree, making fast-import\n>> fail. To avoid this, the temporary branches are tracked and then removed\n>> at the end of the import process.\n>>\n>> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>\n>> ---\n>\n> It might make sense to squash 1/3 with this patch; the definition alone\n> without any actual user would not be very useful and won't help hunting\n> for breakages, if exists, in the future.\n\nMakes sense. WIll correct as you suggest in v3.\n\n>> @@ -2012,7 +2014,27 @@ class P4Sync(Command, P4UserMap):\n>>                              parent = self.initialParents[branch]\n>>                              del self.initialParents[branch]\n>>\n>> -                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)\n>> +                        parentFound = False\n>> +                        if len(parent) > 0:\n>> +                            tempBranch = os.path.join(self.tempBranchLocation, \"%d\" % (change))\n>> +                            if self.verbose:\n>> +                                print \"Creating temporary branch: \" + tempBranch\n>> +                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])\n>> +                            self.tempBranches.append(tempBranch)\n>> +                            self.checkpoint()\n>> +                            for blob in read_pipe_lines(\"git rev-list --reverse --no-merges %s\" % parent):\n>> +                                blob = blob.strip()\n>> +                                if len( read_pipe(\"git diff-tree %s %s\" % (blob, tempBranch)) ) == 0:\n>> +                                    parentFound = True\n>> +                                    if self.verbose:\n>> +                                        print \"Found parent of %s in commit %s\" % (branch, blob)\n>\n> ... also this looks excessively deeply nested, which is a sign that it\n> might be a good idea to refactor this part into a separate helper function\n> or something.\n\nI have to agree with you. Since v3 is required, I'll also include this\nfix along.\n\nThanks,\nVitor\n"},{"id":"182963","messageId":"CAOpHH-W1LY3Q50otrcNJTYWN67k_pCZHEOkgbKy7kPgfUbGeQw@mail.gmail.com","threadId":"29414","inReplyTo":"20120121171130.GA6235@padd.com","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-23T14:01:14Z","receivedAt":"2012-01-23T14:01:14Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"On Sat, Jan 21, 2012 at 5:11 PM, Pete Wyckoff <pw@padd.com> wrote:\n> luke@diamand.org wrote on Sat, 21 Jan 2012 10:51 +0000:\n>> However, one thing I noticed in reading through is that it will\n>> break if you end up importing a P4 branch that has spaces (or other\n>> shell chars) in its name. A quick test confirms this.\n>>\n>> - the code doesn't handle the names properly\n>> - git and p4 have different ideas about valid branch names\n>>\n>> But before rejecting Vitor's changes because of that it would be\n>> worth considering whether we care (much). My own opinion is that if\n>> you have developers who are daft enough to put spaces or dollars in\n>> their branch names then their project is already doomed anyway....\n>>\n>> Perhaps it would be enough just to issue a warning (\"your project is\n>> doomed; start working on your CV\") and skip such branch names rather\n>> than falling over with inexplicable error messages.\n>\n> This doesn't seem like a big deal.  The read_pipe and\n> read_pipe_lines calls shoud be list-ified.  That gets rid\n> of the problem with shell interactions.\n>\n> For git branch name reserved characters, a little function\n> to replace the bogus characters with \"_\" would avoid needing\n> to go work on the resume.  Anything in bad_ref_char() and\n> check_refname_component().  I agree this doesn't have to be\n> perfect.\n>\n> This could be a new patch unrelated to Vitor's series, which\n> verifies branch names anywhere a new commit is made.\n\nI would also prefer to include that fix on a separate patch series that\nwould include the test case Luke already prepared. In my opinion,\nupdating read_pipe and read_pipe_lines is out of scope for the current\npatch series.\n\nBTW, and on an unrelated topic, are any test cases failing on your side?\n\nThanks,\nVitor\n"},{"id":"182992","messageId":"20120123224012.GA10626@padd.com","threadId":"29414","inReplyTo":"CAOpHH-W1LY3Q50otrcNJTYWN67k_pCZHEOkgbKy7kPgfUbGeQw@mail.gmail.com","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-01-23T22:40:12Z","receivedAt":"2012-01-23T22:40:12Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"vitor.hda@gmail.com wrote on Mon, 23 Jan 2012 14:01 +0000:\n> On Sat, Jan 21, 2012 at 5:11 PM, Pete Wyckoff <pw@padd.com> wrote:\n> > luke@diamand.org wrote on Sat, 21 Jan 2012 10:51 +0000:\n> >> However, one thing I noticed in reading through is that it will\n> >> break if you end up importing a P4 branch that has spaces (or other\n> >> shell chars) in its name. A quick test confirms this.\n> >>\n> >> - the code doesn't handle the names properly\n> >> - git and p4 have different ideas about valid branch names\n> >>\n> >> But before rejecting Vitor's changes because of that it would be\n> >> worth considering whether we care (much). My own opinion is that if\n> >> you have developers who are daft enough to put spaces or dollars in\n> >> their branch names then their project is already doomed anyway....\n> >>\n> >> Perhaps it would be enough just to issue a warning (\"your project is\n> >> doomed; start working on your CV\") and skip such branch names rather\n> >> than falling over with inexplicable error messages.\n> >\n> > This doesn't seem like a big deal.  The read_pipe and\n> > read_pipe_lines calls shoud be list-ified.  That gets rid\n> > of the problem with shell interactions.\n> >\n> > For git branch name reserved characters, a little function\n> > to replace the bogus characters with \"_\" would avoid needing\n> > to go work on the resume.  Anything in bad_ref_char() and\n> > check_refname_component().  I agree this doesn't have to be\n> > perfect.\n> >\n> > This could be a new patch unrelated to Vitor's series, which\n> > verifies branch names anywhere a new commit is made.\n> \n> I would also prefer to include that fix on a separate patch series that\n> would include the test case Luke already prepared. In my opinion,\n> updating read_pipe and read_pipe_lines is out of scope for the current\n> patch series.\n\nHow about taking what's below and just squashing it in.  It's\nincremental on your changes and would go well with Luke's series\nthat fixes a bunch of scattered quoting issues similarly.\n\nThe change to \"describe %s\" is unnecessary, but makes all the\ninvocations look similar.  You can leave it out.\n\nThis may conflict if you've already factored out the big\n\"if self.detectBranches\" chunk into a separate function as\nJunio recommended.\n\n> BTW, and on an unrelated topic, are any test cases failing on your side?\n\nI do run the tests regularly, and your series is good.  There's\nthe 'clone --use-client-spec' one that is broken until my\n2ea09b5 (git-p4: adjust test to adhere to stricter useClientSpec,\n2012-01-11) is merged.  It's on pu.\n\n\t\t-- Pete\n\n\n-----------8<-------------------\nFrom f1cfb3836f5150dca86238225da56fe0bd577df8 Mon Sep 17 00:00:00 2001\nFrom: Pete Wyckoff <pw@padd.com>\nDate: Thu, 10 Nov 2011 07:40:14 -0500\nSubject: [PATCH] git-p4: use list invoctaions to avoid shell mangling\n\nChange git and p4 command invocations to avoid going through\nthe shell.  This allows branch names with spaces and wildcards\nto work.\n\nSigned-off-by: Pete Wyckoff <pw@padd.com>\n---\n contrib/fast-import/git-p4 |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4\nindex 2e3b741..b440966 100755\n--- a/contrib/fast-import/git-p4\n+++ b/contrib/fast-import/git-p4\n@@ -1961,7 +1961,7 @@ class P4Sync(Command, P4UserMap):\n     def importChanges(self, changes):\n         cnt = 1\n         for change in changes:\n-            description = p4Cmd(\"describe %s\" % change)\n+            description = p4Cmd([\"describe\", str(change)])\n             self.updateOptionDict(description)\n \n             if not self.silent:\n@@ -2022,9 +2022,9 @@ class P4Sync(Command, P4UserMap):\n                             self.commit(description, filesForCommit, tempBranch, [branchPrefix])\n                             self.tempBranches.append(tempBranch)\n                             self.checkpoint()\n-                            for blob in read_pipe_lines(\"git rev-list --reverse --no-merges %s\" % parent):\n+                            for blob in read_pipe_lines([\"git\", \"rev-list\", \"--reverse\", \"--no-merges\", parent]):\n                                 blob = blob.strip()\n-                                if len( read_pipe(\"git diff-tree %s %s\" % (blob, tempBranch)) ) == 0:\n+                                if len(read_pipe([\"git\", \"diff-tree\", blob, tempBranch])) == 0:\n                                     parentFound = True\n                                     if self.verbose:\n                                         print \"Found parent of %s in commit %s\" % (branch, blob)\n-- \n1.7.9.rc2.33.g492ae\n"},{"id":"183057","messageId":"CAOpHH-V2nZ8meh7x6vCVGUQCKQqJ+sPcnGRo+8SqfNavg7F87w@mail.gmail.com","threadId":"29414","inReplyTo":"20120123224012.GA10626@padd.com","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-25T01:23:31Z","receivedAt":"2012-01-25T01:23:31Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"On Mon, Jan 23, 2012 at 10:40 PM, Pete Wyckoff <pw@padd.com> wrote:\n> How about taking what's below and just squashing it in.  It's\n> incremental on your changes and would go well with Luke's series\n> that fixes a bunch of scattered quoting issues similarly.\n>\n> The change to \"describe %s\" is unnecessary, but makes all the\n> invocations look similar.  You can leave it out.\n\nI've squashed your patch, but kept the \"describe %s\" fix in a separate\ncommit.\n\n>> BTW, and on an unrelated topic, are any test cases failing on your side?\n>\n> I do run the tests regularly, and your series is good.  There's\n> the 'clone --use-client-spec' one that is broken until my\n> 2ea09b5 (git-p4: adjust test to adhere to stricter useClientSpec,\n> 2012-01-11) is merged.  It's on pu.\n\nTests in t9809-git-p4-client-view.sh were failing for me because I'm\nusing dash instead of bash. Please check patch below for a fix.\n\nTest 15 of t9800-git-p4-basic.sh is still failing and I've not been able\nto pinpoint the problem. I can send you the logs off-list, if you want.\n\nThanks,\nVitor\n\n\n\ndiff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh\nindex c9471d5..5b0ad99 100755\n--- a/t/t9809-git-p4-client-view.sh\n+++ b/t/t9809-git-p4-client-view.sh\n@@ -31,7 +31,7 @@ client_view() {\n #\n check_files_exist() {\n        ok=0 &&\n-       num=${#@} &&\n+       num=$# &&\n        for arg ; do\n                test_path_is_file \"$arg\" &&\n                ok=$(($ok + 1))\n"},{"id":"183060","messageId":"CAOpHH-UxD37v7N3U9A0c_MnzSjOcF6eJCx2WdHRKf2CFoYy_tg@mail.gmail.com","threadId":"29414","inReplyTo":"CAOpHH-Wcf3innjA4LS0TMrLzEwbQzfZmHssxSBYvv4v7UMfi1w@mail.gmail.com","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Vitor Antunes","fromEmail":"vitor.hda@gmail.com","sentAt":"2012-01-25T01:39:11Z","receivedAt":"2012-01-25T01:39:11Z","isPatch":true,"sender":{"key":"vitor.hda@gmail.com","avatar":"https://avatars.githubusercontent.com/u/606876?v=4"},"body":"On Mon, Jan 23, 2012 at 1:53 PM, Vitor Antunes <vitor.hda@gmail.com> wrote:\n> On Sat, Jan 21, 2012 at 4:54 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Vitor Antunes <vitor.hda@gmail.com> writes:\n>>\n>>> +test_expect_success 'git-p4 add complex branches' '\n>>> +     test_when_finished cleanup_git &&\n>>> +     test_create_repo \"$git\" &&\n>>> +     (\n>>> +             cd \"$cli\" &&\n>>> +             changelist=$(p4 changes -m1 //depot/... | cut -d\" \" -f2) &&\n>>> +             changelist=$(($changelist - 5)) &&\n>>> +             p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&\n>>> +             p4 submit -d \"branch4\" &&\n>>> +             changelist=$(($changelist + 2)) &&\n>>> +             p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&\n>>> +             p4 submit -d \"branch5\"\n>>\n>> That's a strange quoting convention. Why are \"branch4\" and \"branch5\"\n>> enclosed in double quotes while \"integrate\" and \"submit\" aren't?\n>> (rhetorical: do not quote these branch names without a good reason).\n>\n> There is no reason that I can remember to have those enclosed in double\n> quotes. Will double check in my local branches at home tonight. Anyway,\n> expect a fix for this in v3.\n\nI now see why I added the quotes. The -d option is used to input the\ndescription of the commit, which can contain spaces and other special\ncharacters. Admittedly they are not required in this case, but from a\nconsistency point of view I would prefer to keep them. Is this\nacceptable?\n\n>>> +# Configure branches through git-config and clone them. git-p4 will only be able\n>>> +# to clone the original structure if it is able to detect the origin changelist\n>>> +# of each branch.\n>>> +test_expect_success 'git-p4 clone complex branches' '\n>>> +     test_when_finished cleanup_git &&\n>>> +     test_create_repo \"$git\" &&\n>>> +     (\n>>> +             cd \"$git\" &&\n>>> +             git config git-p4.branchList branch1:branch2 &&\n>>> +             git config --add git-p4.branchList branch1:branch3 &&\n>>> +             git config --add git-p4.branchList branch1:branch4 &&\n>>> +             git config --add git-p4.branchList branch1:branch5 &&\n>>> +             \"$GITP4\" clone --dest=. --detect-branches //depot@all &&\n>>> +             git log --all --graph --decorate --stat &&\n>>> +             git reset --hard p4/depot/branch1 &&\n>>> +             test_path_is_file file1 &&\n>>> +             test_path_is_file file2 &&\n>>> +             test_path_is_file file3 &&\n>>> +             grep -q update file2 &&\n>>\n>> Do you really need to use \"-q\" here?  Wouldn't it help if you wrote it\n>> without it while debugging tests with \"sh ./t9801-*.sh -v\"?\n>\n> Makes sense.\n>\n> Thank you for the helpful comments.\n> Vitor\n\nApparently I've hit \"Reply\" instead of \"Reply all\" before, so I'll keep\nthe full quote of my previous email for future reference.\n\nVitor\n"},{"id":"183064","messageId":"7vwr8g1lbx.fsf@alter.siamese.dyndns.org","threadId":"29414","inReplyTo":"CAOpHH-UxD37v7N3U9A0c_MnzSjOcF6eJCx2WdHRKf2CFoYy_tg@mail.gmail.com","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-25T04:02:10Z","receivedAt":"2012-01-25T04:02:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vitor Antunes <vitor.hda@gmail.com> writes:\n\n>>>> +             p4 submit -d \"branch5\"\n>>>\n>>> That's a strange quoting convention. Why are \"branch4\" and \"branch5\"\n>>> enclosed in double quotes while \"integrate\" and \"submit\" aren't?\n>>> (rhetorical: do not quote these branch names without a good reason).\n>>\n>> There is no reason that I can remember to have those enclosed in double\n>> quotes. Will double check in my local branches at home tonight. Anyway,\n>> expect a fix for this in v3.\n>\n> I now see why I added the quotes. The -d option is used to input the\n> description of the commit, which can contain spaces and other special\n> characters. Admittedly they are not required in this case, but from a\n> consistency point of view I would prefer to keep them.\n\nHmm, the argument \"branch5\" made it look like it is the name of the branch\nyou are giving here. If it is supposed to be human-readable free-form text\ndescription, I would prefer to see it as such, e.g.\n\n\tp4 submit -d \"integrate changes on branch #1 to branch #5\"\n\nor something like that.\n"},{"id":"183079","messageId":"20120125123425.GA3406@padd.com","threadId":"29414","inReplyTo":"CAOpHH-V2nZ8meh7x6vCVGUQCKQqJ+sPcnGRo+8SqfNavg7F87w@mail.gmail.com","subject":"Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import","fromName":"Pete Wyckoff","fromEmail":"pw@padd.com","sentAt":"2012-01-25T12:34:25Z","receivedAt":"2012-01-25T12:34:25Z","isPatch":true,"sender":{"key":"pw@padd.com","avatar":null},"body":"vitor.hda@gmail.com wrote on Wed, 25 Jan 2012 01:23 +0000:\n> On Mon, Jan 23, 2012 at 10:40 PM, Pete Wyckoff <pw@padd.com> wrote:\n> > How about taking what's below and just squashing it in.  It's\n> > incremental on your changes and would go well with Luke's series\n> > that fixes a bunch of scattered quoting issues similarly.\n> >\n> > The change to \"describe %s\" is unnecessary, but makes all the\n> > invocations look similar.  You can leave it out.\n> \n> I've squashed your patch, but kept the \"describe %s\" fix in a separate\n> commit.\n> \n> >> BTW, and on an unrelated topic, are any test cases failing on your side?\n> >\n> > I do run the tests regularly, and your series is good.  There's\n> > the 'clone --use-client-spec' one that is broken until my\n> > 2ea09b5 (git-p4: adjust test to adhere to stricter useClientSpec,\n> > 2012-01-11) is merged.  It's on pu.\n> \n> Tests in t9809-git-p4-client-view.sh were failing for me because I'm\n> using dash instead of bash. Please check patch below for a fix.\n> \n> Test 15 of t9800-git-p4-basic.sh is still failing and I've not been able\n> to pinpoint the problem. I can send you the logs off-list, if you want.\n> \n> Thanks,\n> Vitor\n> \n> \n> \n> diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh\n> index c9471d5..5b0ad99 100755\n> --- a/t/t9809-git-p4-client-view.sh\n> +++ b/t/t9809-git-p4-client-view.sh\n> @@ -31,7 +31,7 @@ client_view() {\n>  #\n>  check_files_exist() {\n>         ok=0 &&\n> -       num=${#@} &&\n> +       num=$# &&\n>         for arg ; do\n>                 test_path_is_file \"$arg\" &&\n>                 ok=$(($ok + 1))\n> \n\nYes, thanks.  Plain old $# works fine, even if the arguments have\nspaces.  I'll hang onto this with some other work in the same\narea and submit it eventually.\n\n\t\t-- Pete\n"}]}