{"thread":{"id":"41040","subject":"[PATCHv2 3/3] git-p4: reduce number of server queries for fetches","startedAt":"2015-12-19T09:39:37Z","lastAt":"2015-12-21T19:27:33Z","messageCount":5,"participants":["Luke Diamand","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"274751","messageId":"1450517980-1744-1-git-send-email-luke@diamand.org","threadId":"41040","inReplyTo":null,"subject":"[PATCHv2 0/3] git-p4: fixing p4ChangesForPaths","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-19T09:39:37Z","receivedAt":"2015-12-19T09:39:37Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"James Farwell found a bug in p4ChangesForPaths() when handling\nchanges across multiple depot paths.\n\nSam Hocevar had already submitted a change to the same function\nto get P4 to do queries across all of the depot paths, in order\nto reduce server queries, which had the side effect of fixing\nJames' problem.\n\nThis followup just fixes Sam's original fix to restore the\nbehavior of p4ChangesForPaths() so that it returns a sorted list\nof changes. That fixes a failing a testcase.\n\nLuke Diamand (1):\n git-p4: failing test case for skipping changes with multiple depots\n\nSam Hocevar (2):\n git-p4: support multiple depot paths in p4 submit\n git-p4: reduce number of server queries for fetches\n\n git-p4.py | 55 +++++++++++++++++++++++++++----------------------\n t/t9818-git-p4-block.sh | 28 ++++++++++++++++++++++++-\n 2 files changed, 57 insertions(+), 26 deletions(-)\n\n-- \n2.6.2.474.g3eb3291\n"},{"id":"274752","messageId":"1450517980-1744-2-git-send-email-luke@diamand.org","threadId":"41040","inReplyTo":"1450517980-1744-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 1/3] git-p4: failing test case for skipping changes with multiple depots","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-19T09:39:38Z","receivedAt":"2015-12-19T09:39:38Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"James Farwell reported that with multiple depots git-p4 would\nskip changes.\n\nhttp://article.gmane.org/gmane.comp.version-control.git/282297\n\nAdd a failing test case demonstrating the problem.\n\nSigned-off-by: Luke Diamand \n---\n t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-\n 1 file changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nindex 3b3ae1f..64510b7 100755\n--- a/t/t9818-git-p4-block.sh\n+++ b/t/t9818-git-p4-block.sh\n@@ -84,7 +84,7 @@ p4_add_file() {\n \t(cd \"$cli\" &&\n \t\t>$1 &&\n \t\tp4 add $1 &&\n-\t\tp4 submit -d \"Added a file\" $1\n+\t\tp4 submit -d \"Added file $1\" $1\n \t)\n }\n \n@@ -112,6 +112,32 @@ test_expect_success 'Syncing files' '\n \t)\n '\n \n+# Handling of multiple depot paths:\n+# git p4 clone //depot/pathA //depot/pathB\n+#\n+test_expect_success 'Create a repo with multiple depot paths' '\n+\tclient_view \"//depot/pathA/... //client/pathA/...\" \\\n+\t\t \"//depot/pathB/... //client/pathB/...\" &&\n+\tmkdir -p \"$cli/pathA\" \"$cli/pathB\" &&\n+\tfor p in pathA pathB\n+\tdo\n+\t\tfor i in $(test_seq 1 10)\n+\t\tdo\n+\t\t\tp4_add_file \"$p/file$p$i\"\n+\t\tdone\n+\tdone\n+'\n+\n+test_expect_failure 'Clone repo with multiple depot paths' '\n+\t(\n+\t\tcd \"$git\" &&\n+\t\tgit p4 clone --changes-block-size=4 //depot/pathA@all //depot/pathB@all \\\n+\t\t\t--destination=dest &&\n+\t\tls -1 dest >log &&\n+\t\ttest_line_count = 20 log\n+\t)\n+'\n+\n test_expect_success 'kill p4d' '\n \tkill_p4d\n '\n-- \n2.6.2.474.g3eb3291\n"},{"id":"274753","messageId":"1450517980-1744-3-git-send-email-luke@diamand.org","threadId":"41040","inReplyTo":"1450517980-1744-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 2/3] git-p4: support multiple depot paths in p4 submit","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-19T09:39:39Z","receivedAt":"2015-12-19T09:39:39Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"From: Sam Hocevar \n\nWhen submitting from a repository that was cloned using a client spec,\nuse the full list of paths when ruling out files that are outside the\nview. This fixes a bug where only files pertaining to the first path\nwould be included in the p4 submit.\n\nSigned-off-by: Sam Hocevar \nSigned-off-by: Luke Diamand \n---\n git-p4.py | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 7a9dd6a..122aff2 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1458,6 +1458,8 @@ class P4Submit(Command, P4UserMap):\n Remove lines in the Files section that show changes to files\n outside the depot path we're committing into.\"\"\"\n \n+ [upstream, settings] = findUpstreamBranchPoint()\n+\n template = \"\"\n inFilesSection = False\n for line in p4_read_pipe_lines(['change', '-o']):\n@@ -1470,8 +1472,13 @@ class P4Submit(Command, P4UserMap):\n lastTab = path.rfind(\"\\t\")\n if lastTab != -1:\n path = path[:lastTab]\n- if not p4PathStartsWith(path, self.depotPath):\n- continue\n+ if settings.has_key('depot-paths'):\n+ if not [p for p in settings['depot-paths']\n+ if p4PathStartsWith(path, p)]:\n+ continue\n+ else:\n+ if not p4PathStartsWith(path, self.depotPath):\n+ continue\n else:\n inFilesSection = False\n else:\n-- \n2.6.2.474.g3eb3291\n"},{"id":"274750","messageId":"1450517980-1744-4-git-send-email-luke@diamand.org","threadId":"41040","inReplyTo":"1450517980-1744-1-git-send-email-luke@diamand.org","subject":"[PATCHv2 3/3] git-p4: reduce number of server queries for fetches","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-19T09:39:40Z","receivedAt":"2015-12-19T09:39:40Z","isPatch":false,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"From: Sam Hocevar \n\nWhen fetching changes from a depot using a full client spec, there\nis no need to perform as many queries as there are top-level paths\nin the client spec. Instead we query all changes in chronological\norder, also getting rid of the need to sort the results and remove\nduplicates.\n\nSigned-off-by: Sam Hocevar \nSigned-off-by: Luke Diamand \n---\n git-p4.py | 44 +++++++++++++++++++++-----------------------\n t/t9818-git-p4-block.sh | 2 +-\n 2 files changed, 22 insertions(+), 24 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 122aff2..c33dece 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -822,39 +822,37 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n die(\"cannot use --changes-block-size with non-numeric revisions\")\n block_size = None\n \n- # Accumulate change numbers in a dictionary to avoid duplicates\n- changes = {}\n+ changes = []\n \n- for p in depotPaths:\n- # Retrieve changes a block at a time, to prevent running\n- # into a MaxResults/MaxScanRows error from the server.\n+ # Retrieve changes a block at a time, to prevent running\n+ # into a MaxResults/MaxScanRows error from the server.\n \n- while True:\n- cmd = ['changes']\n+ while True:\n+ cmd = ['changes']\n \n- if block_size:\n- end = min(changeEnd, changeStart + block_size)\n- revisionRange = \"%d,%d\" % (changeStart, end)\n- else:\n- revisionRange = \"%s,%s\" % (changeStart, changeEnd)\n+ if block_size:\n+ end = min(changeEnd, changeStart + block_size)\n+ revisionRange = \"%d,%d\" % (changeStart, end)\n+ else:\n+ revisionRange = \"%s,%s\" % (changeStart, changeEnd)\n \n+ for p in depotPaths:\n cmd += [\"%s...@%s\" % (p, revisionRange)]\n \n- for line in p4_read_pipe_lines(cmd):\n- changeNum = int(line.split(\" \")[1])\n- changes[changeNum] = True\n+ # Insert changes in chronological order\n+ for line in reversed(p4_read_pipe_lines(cmd)):\n+ changes.append(int(line.split(\" \")[1]))\n \n- if not block_size:\n- break\n+ if not block_size:\n+ break\n \n- if end >= changeEnd:\n- break\n+ if end >= changeEnd:\n+ break\n \n- changeStart = end + 1\n+ changeStart = end + 1\n \n- changelist = changes.keys()\n- changelist.sort()\n- return changelist\n+ changes = sorted(changes)\n+ return changes\n \n def p4PathStartsWith(path, prefix):\n # This method tries to remedy a potential mixed-case issue:\ndiff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh\nindex 64510b7..8840a18 100755\n--- a/t/t9818-git-p4-block.sh\n+++ b/t/t9818-git-p4-block.sh\n@@ -128,7 +128,7 @@ test_expect_success 'Create a repo with multiple depot paths' '\n \tdone\n '\n \n-test_expect_failure 'Clone repo with multiple depot paths' '\n+test_expect_success 'Clone repo with multiple depot paths' '\n \t(\n \t\tcd \"$git\" &&\n \t\tgit p4 clone --changes-block-size=4 //depot/pathA@all //depot/pathB@all \\\n-- \n2.6.2.474.g3eb3291\n"},{"id":"274819","messageId":"xmqqlh8nfvq2.fsf@gitster.mtv.corp.google.com","threadId":"41040","inReplyTo":"1450517980-1744-1-git-send-email-luke@diamand.org","subject":"Re: [PATCHv2 0/3] git-p4: fixing p4ChangesForPaths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T19:27:33Z","receivedAt":"2015-12-21T19:27:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will replace what was queued on 'pu'.\n"}]}