{"thread":{"id":"40987","subject":"[PATCH 0/2] git-p4: fix for handling of multiple depot paths","startedAt":"2015-12-13T20:07:12Z","lastAt":"2015-12-16T07:51:39Z","messageCount":11,"participants":["Luke Diamand","Junio C Hamano","James Farwell","Sam Hocevar"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"274363","messageId":"1450037234-15344-1-git-send-email-luke@diamand.org","threadId":"40987","inReplyTo":null,"subject":"[PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-13T20:07:12Z","receivedAt":"2015-12-13T20:07:12Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"James Farwell reported a bug I introduced into git-p4 with\nhandling of multiple depot paths:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/282297\n\nThis patch series adds a failing test case, and a fix for this\nproblem.\n\nLuke\n\nLuke Diamand (2):\n git-p4: failing test case for skipping changes with multiple depots\n git-p4: fix handling of multiple depot paths\n\n git-p4.py | 8 +++++---\n t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-\n 2 files changed, 32 insertions(+), 4 deletions(-)\n\n-- \n2.6.2.474.g3eb3291\n"},{"id":"274364","messageId":"1450037234-15344-2-git-send-email-luke@diamand.org","threadId":"40987","inReplyTo":"1450037234-15344-1-git-send-email-luke@diamand.org","subject":"[PATCH 1/2] git-p4: failing test case for skipping changes with multiple depots","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-13T20:07:13Z","receivedAt":"2015-12-13T20:07:13Z","isPatch":true,"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":"274365","messageId":"1450037234-15344-3-git-send-email-luke@diamand.org","threadId":"40987","inReplyTo":"1450037234-15344-1-git-send-email-luke@diamand.org","subject":"[PATCH 2/2] git-p4: fix handling of multiple depot paths","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-13T20:07:14Z","receivedAt":"2015-12-13T20:07:14Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"With multiple depot paths (//depot/pathA, //depot/pathB) if there\nare more changes than the changes-block-size limit, then some\nof the changes will be skipped. This fixes this by correcting\nthe loop in p4ChangesForPaths() to reset the \"start\" point\nfor each depot.\n\nSuggested-by: James Farwell \nSigned-off-by: Luke Diamand \n---\n git-p4.py | 8 +++++---\n t/t9818-git-p4-block.sh | 2 +-\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex 7a9dd6a..a8b5278 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -829,12 +829,14 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n # Retrieve changes a block at a time, to prevent running\n # into a MaxResults/MaxScanRows error from the server.\n \n+ start = changeStart\n+\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+ end = min(changeEnd, start + block_size)\n+ revisionRange = \"%d,%d\" % (start, end)\n else:\n revisionRange = \"%s,%s\" % (changeStart, changeEnd)\n \n@@ -850,7 +852,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):\n if end >= changeEnd:\n break\n \n- changeStart = end + 1\n+ start = end + 1\n \n changelist = changes.keys()\n changelist.sort()\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":"274366","messageId":"CAE5ih7_T1xC9AyO41T4ktJmj6tENaEGbAG556WLyfsYz-jawsw@mail.gmail.com","threadId":"40987","inReplyTo":"1450037234-15344-1-git-send-email-luke@diamand.org","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-13T20:19:42Z","receivedAt":"2015-12-13T20:19:42Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Having just fixed this, I've now just spotted that Sam Hocevar's fix\nto reduce the number of P4 transactions also fixes it:\n\nhttps://www.mail-archive.com/git%40vger.kernel.org/msg81880.html\n\nThat seems like a cleaner fix.\n\nLuke\n\n\nOn 13 December 2015 at 20:07, Luke Diamand wrote:\n> James Farwell reported a bug I introduced into git-p4 with\n> handling of multiple depot paths:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/282297\n>\n> This patch series adds a failing test case, and a fix for this\n> problem.\n>\n> Luke\n>\n> Luke Diamand (2):\n> git-p4: failing test case for skipping changes with multiple depots\n> git-p4: fix handling of multiple depot paths\n>\n> git-p4.py | 8 +++++---\n> t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-\n> 2 files changed, 32 insertions(+), 4 deletions(-)\n>\n> --\n> 2.6.2.474.g3eb3291\n>\n"},{"id":"274396","messageId":"xmqqio40kfhl.fsf@gitster.mtv.corp.google.com","threadId":"40987","inReplyTo":"CAE5ih7_T1xC9AyO41T4ktJmj6tENaEGbAG556WLyfsYz-jawsw@mail.gmail.com","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-14T19:16:38Z","receivedAt":"2015-12-14T19:16:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> Having just fixed this, I've now just spotted that Sam Hocevar's fix\n> to reduce the number of P4 transactions also fixes it:\n>\n> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html\n>\n> That seems like a cleaner fix.\n\nHmm, do you mean I should ignore this series and take the other one,\ntake only 1/2 from this for tests and then both patches in the other\none, or something else?\n\nThanks.\n\n>\n> Luke\n>\n>\n> On 13 December 2015 at 20:07, Luke Diamand wrote:\n>> James Farwell reported a bug I introduced into git-p4 with\n>> handling of multiple depot paths:\n>>\n>> http://article.gmane.org/gmane.comp.version-control.git/282297\n>>\n>> This patch series adds a failing test case, and a fix for this\n>> problem.\n>>\n>> Luke\n>>\n>> Luke Diamand (2):\n>> git-p4: failing test case for skipping changes with multiple depots\n>> git-p4: fix handling of multiple depot paths\n>>\n>> git-p4.py | 8 +++++---\n>> t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-\n>> 2 files changed, 32 insertions(+), 4 deletions(-)\n>>\n>> --\n>> 2.6.2.474.g3eb3291\n>>\n"},{"id":"274426","messageId":"CAE5ih7_9m8kw=sVj8Sv5mAfT_22-g0vdTb78FvLTrNUkJO0M0g@mail.gmail.com","threadId":"40987","inReplyTo":"xmqqio40kfhl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-14T20:58:06Z","receivedAt":"2015-12-14T20:58:06Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 14 December 2015 at 19:16, Junio C Hamano wrote:\n> Luke Diamand writes:\n>\n>> Having just fixed this, I've now just spotted that Sam Hocevar's fix\n>> to reduce the number of P4 transactions also fixes it:\n>>\n>> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html\n>>\n>> That seems like a cleaner fix.\n>\n> Hmm, do you mean I should ignore this series and take the other one,\n> take only 1/2 from this for tests and then both patches in the other\n> one, or something else?\n\nThe second of those (take only 1/2 from this for tests, and then both\nfrom the other) seems like the way to go.\n\nThanks,\nLuke\n"},{"id":"274443","messageId":"xmqqtwnkhegw.fsf@gitster.mtv.corp.google.com","threadId":"40987","inReplyTo":"CAE5ih7_9m8kw=sVj8Sv5mAfT_22-g0vdTb78FvLTrNUkJO0M0g@mail.gmail.com","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-14T22:06:55Z","receivedAt":"2015-12-14T22:06:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> On 14 December 2015 at 19:16, Junio C Hamano wrote:\n>> Luke Diamand writes:\n>>\n>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix\n>>> to reduce the number of P4 transactions also fixes it:\n>>>\n>>> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html\n>>>\n>>> That seems like a cleaner fix.\n>>\n>> Hmm, do you mean I should ignore this series and take the other one,\n>> take only 1/2 from this for tests and then both patches in the other\n>> one, or something else?\n>\n> The second of those (take only 1/2 from this for tests, and then both\n> from the other) seems like the way to go.\n\nOK. Should I consider the two patches from Sam \"Reviewed-by\" you?\n"},{"id":"274462","messageId":"CAE5ih7_qY5oF+UWs4gE2eHUu17pBg6TVGTUyRRRcBe12ybkw+Q@mail.gmail.com","threadId":"40987","inReplyTo":"xmqqtwnkhegw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-14T23:09:21Z","receivedAt":"2015-12-14T23:09:21Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"Sorry - I've just run the tests, and this change causes one of the\ntest cases in t9800-git-p4-basic.sh to fail.\n\nIt looks like the test case makes an assumption about who wins if two\nP4 depots have changes to files that end up in the same place, and\nthis change reverses the order. It may actually be fine, but it needs\nto be thought about a bit.\n\nSam - do you have any thoughts on this?\n\nThanks\nLuke\n\n\n\n\n\n\nOn 14 December 2015 at 22:06, Junio C Hamano wrote:\n> Luke Diamand writes:\n>\n>> On 14 December 2015 at 19:16, Junio C Hamano wrote:\n>>> Luke Diamand writes:\n>>>\n>>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix\n>>>> to reduce the number of P4 transactions also fixes it:\n>>>>\n>>>> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html\n>>>>\n>>>> That seems like a cleaner fix.\n>>>\n>>> Hmm, do you mean I should ignore this series and take the other one,\n>>> take only 1/2 from this for tests and then both patches in the other\n>>> one, or something else?\n>>\n>> The second of those (take only 1/2 from this for tests, and then both\n>> from the other) seems like the way to go.\n>\n> OK. Should I consider the two patches from Sam \"Reviewed-by\" you?\n"},{"id":"274517","messageId":"1450220213834.32062@vmware.com","threadId":"40987","inReplyTo":"CAE5ih7_qY5oF+UWs4gE2eHUu17pBg6TVGTUyRRRcBe12ybkw+Q@mail.gmail.com","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"James Farwell","fromEmail":"jfarwell@vmware.com","sentAt":"2015-12-15T22:56:38Z","receivedAt":"2015-12-15T22:56:38Z","isPatch":true,"sender":{"key":"jfarwell@vmware.com","avatar":"https://gravatar.com/avatar/ab0d9202e2b5b9267d4c1cc792bbe3b00945504feeef83b0633770346fc71c7d?d=mp&s=160"},"body":"I'm not sure if my opinion as an outsider is of use, but since the perforce change number is monotonically increasing, my expectation as a user would be for them to be applied in order by the perforce change number. :)\n\n- James\n\n________________________________________\nFrom: Luke Diamand \nSent: Monday, December 14, 2015 3:09 PM\nTo: Junio C Hamano\nCc: Git Users; James Farwell; Lars Schneider; Eric Sunshine; Sam Hocevar\nSubject: Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths\n\nSorry - I've just run the tests, and this change causes one of the\ntest cases in t9800-git-p4-basic.sh to fail.\n\nIt looks like the test case makes an assumption about who wins if two\nP4 depots have changes to files that end up in the same place, and\nthis change reverses the order. It may actually be fine, but it needs\nto be thought about a bit.\n\nSam - do you have any thoughts on this?\n\nThanks\nLuke\n\n\n\n\n\n\nOn 14 December 2015 at 22:06, Junio C Hamano wrote:\n> Luke Diamand writes:\n>\n>> On 14 December 2015 at 19:16, Junio C Hamano wrote:\n>>> Luke Diamand writes:\n>>>\n>>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix\n>>>> to reduce the number of P4 transactions also fixes it:\n>>>>\n>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_git-2540vger.kernel.org_msg81880.html&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=wkCayFhpIBdAOEa7tZDTcd1weqwtiFMEIQTL-WQPwC4&m=q8dsOAHvUiDzzPNGRAfMMrcXstxNlI-v7I_03uEL1e8&s=C8wVLMC-iU7We0r36sxOuu920ZjZYdpy7ysNi_5PYv8&e=\n>>>>\n>>>> That seems like a cleaner fix.\n>>>\n>>> Hmm, do you mean I should ignore this series and take the other one,\n>>> take only 1/2 from this for tests and then both patches in the other\n>>> one, or something else?\n>>\n>> The second of those (take only 1/2 from this for tests, and then both\n>> from the other) seems like the way to go.\n>\n> OK. Should I consider the two patches from Sam \"Reviewed-by\" you?"},{"id":"274539","messageId":"20151216003834.GG48528@hocevar.net","threadId":"40987","inReplyTo":"1450220213834.32062@vmware.com","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Sam Hocevar","fromEmail":"sam@hocevar.net","sentAt":"2015-12-16T00:38:34Z","receivedAt":"2015-12-16T00:38:34Z","isPatch":true,"sender":{"key":"sam@hocevar.net","avatar":"https://avatars.githubusercontent.com/u/245089?v=4"},"body":" I'm actually surprised that the patch changes the order at all, since\nall it does is affect the decision (on a yes/no basis) to include a given\nfile into a changelist. I'm going to have a look at that specific unit\ntest, but of course as a user I'd prefer if the default behaviour could\nremain the same, unless it was actually a bug.\n\n-- \nSam.\n\nOn Tue, Dec 15, 2015, James Farwell wrote:\n> I'm not sure if my opinion as an outsider is of use, but since the perforce change number is monotonically increasing, my expectation as a user would be for them to be applied in order by the perforce change number. :)\n> \n> - James\n> \n> ________________________________________\n> From: Luke Diamand \n> Sent: Monday, December 14, 2015 3:09 PM\n> To: Junio C Hamano\n> Cc: Git Users; James Farwell; Lars Schneider; Eric Sunshine; Sam Hocevar\n> Subject: Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths\n> \n> Sorry - I've just run the tests, and this change causes one of the\n> test cases in t9800-git-p4-basic.sh to fail.\n> \n> It looks like the test case makes an assumption about who wins if two\n> P4 depots have changes to files that end up in the same place, and\n> this change reverses the order. It may actually be fine, but it needs\n> to be thought about a bit.\n> \n> Sam - do you have any thoughts on this?\n> \n> Thanks\n> Luke\n> \n> \n> \n> \n> \n> \n> On 14 December 2015 at 22:06, Junio C Hamano wrote:\n> > Luke Diamand writes:\n> >\n> >> On 14 December 2015 at 19:16, Junio C Hamano wrote:\n> >>> Luke Diamand writes:\n> >>>\n> >>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix\n> >>>> to reduce the number of P4 transactions also fixes it:\n> >>>>\n> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_git-2540vger.kernel.org_msg81880.html&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=wkCayFhpIBdAOEa7tZDTcd1weqwtiFMEIQTL-WQPwC4&m=q8dsOAHvUiDzzPNGRAfMMrcXstxNlI-v7I_03uEL1e8&s=C8wVLMC-iU7We0r36sxOuu920ZjZYdpy7ysNi_5PYv8&e=\n> >>>>\n> >>>> That seems like a cleaner fix.\n> >>>\n> >>> Hmm, do you mean I should ignore this series and take the other one,\n> >>> take only 1/2 from this for tests and then both patches in the other\n> >>> one, or something else?\n> >>\n> >> The second of those (take only 1/2 from this for tests, and then both\n> >> from the other) seems like the way to go.\n> >\n> > OK. Should I consider the two patches from Sam \"Reviewed-by\" you?\necho \"creationism\" | tr -d \"holy godly goal\"\n"},{"id":"274556","messageId":"5671180B.9010008@diamand.org","threadId":"40987","inReplyTo":"20151216003834.GG48528@hocevar.net","subject":"Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-16T07:51:39Z","receivedAt":"2015-12-16T07:51:39Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 16/12/15 00:38, Sam Hocevar wrote:\n> I'm actually surprised that the patch changes the order at all,\n> since all it does is affect the decision (on a yes/no basis) to\n> include a given file into a changelist. I'm going to have a look at\n> that specific unit test, but of course as a user I'd prefer if the\n> default behaviour could remain the same, unless it was actually a\n> bug.\n>\n\nWe ask for changes in\n //depot/sub1/...@1,6\n //depot/sub2/...@1,6'\n\nwhich gives us [4, 6, 3, 5].\n\nThe old code used to sort this list but this change removes the sort.\nMaybe putting the sort back would fix it?\n\n> I'm not sure if my opinion as an outsider is of use, but since the\n> perforce change number is monotonically increasing, my expectation as\n> a user would be for them to be applied in order by the perforce\n> change number.\n\nIn answer to James' question, the test checks that the most recent\nchange wins (i.e. applied in order).\n\nLuke\n"}]}