threads / patch / 40987

patch, 2 partsgit-p4: fix for handling of multiple depot paths

Subject: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

## tl;dr

11 messages between Dec 13, 2015 and Dec 16, 2015. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Luke Diamand· Dec 13, 2015, 20:07 UTC · lore

James Farwell reported a bug I introduced into git-p4 with handling of multiple depot paths:

http://article.gmane.org/gmane.comp.version-control.git/282297

This patch series adds a failing test case, and a fix for this problem.

Luke
Luke Diamand (2):
  git-p4: failing test case for skipping changes with multiple depots
  git-p4: fix handling of multiple depot paths
 git-p4.py               |  8 +++++---
 t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 4 deletions(-)
-- 
2.6.2.474.g3eb3291
Luke Diamand· Dec 13, 2015, 20:07 UTC · re: Luke Diamand · lore

[PATCH 1/2] git-p4: failing test case for skipping changes with multiple depots

James Farwell reported that with multiple depots git-p4 would skip changes.

http://article.gmane.org/gmane.comp.version-control.git/282297
Add a failing test case demonstrating the problem.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)
Show changes to t/t9818-git-p4-block.sh +27 −1
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 3b3ae1f..64510b7 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -84,7 +84,7 @@ p4_add_file() {
 	(cd "$cli" &&
 		>$1 &&
 		p4 add $1 &&
-		p4 submit -d "Added a file" $1
+		p4 submit -d "Added file $1" $1
 	)
 }
 
@@ -112,6 +112,32 @@ test_expect_success 'Syncing files' '
 	)
 '
 
+# Handling of multiple depot paths:
+#    git p4 clone //depot/pathA //depot/pathB
+#
+test_expect_success 'Create a repo with multiple depot paths' '
+	client_view "//depot/pathA/... //client/pathA/..." \
+		    "//depot/pathB/... //client/pathB/..." &&
+	mkdir -p "$cli/pathA" "$cli/pathB" &&
+	for p in pathA pathB
+	do
+		for i in $(test_seq 1 10)
+		do
+			p4_add_file "$p/file$p$i"
+		done
+	done
+'
+
+test_expect_failure 'Clone repo with multiple depot paths' '
+	(
+		cd "$git" &&
+		git p4 clone --changes-block-size=4 //depot/pathA@all //depot/pathB@all \
+			--destination=dest &&
+		ls -1 dest >log &&
+		test_line_count = 20 log
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '

-- 
2.6.2.474.g3eb3291
Luke Diamand· Dec 13, 2015, 20:07 UTC · re: Luke Diamand · lore

[PATCH 2/2] git-p4: fix handling of multiple depot paths

With multiple depot paths (//depot/pathA, //depot/pathB) if there are more changes than the changes-block-size limit, then some of the changes will be skipped. This fixes this by correcting the loop in p4ChangesForPaths() to reset the "start" point for each depot.

Suggested-by: James Farwell <jfarwell@vmware.com>
Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py               | 8 +++++---
 t/t9818-git-p4-block.sh | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)
Show changes to 2 files +6 −4

git-p4.py, t/t9818-git-p4-block.sh

diff --git a/git-p4.py b/git-p4.py
index 7a9dd6a..a8b5278 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -829,12 +829,14 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
         # Retrieve changes a block at a time, to prevent running
         # into a MaxResults/MaxScanRows error from the server.
 
+        start = changeStart
+
         while True:
             cmd = ['changes']
 
             if block_size:
-                end = min(changeEnd, changeStart + block_size)
-                revisionRange = "%d,%d" % (changeStart, end)
+                end = min(changeEnd, start + block_size)
+                revisionRange = "%d,%d" % (start, end)
             else:
                 revisionRange = "%s,%s" % (changeStart, changeEnd)
 
@@ -850,7 +852,7 @@ def p4ChangesForPaths(depotPaths, changeRange, requestedBlockSize):
             if end >= changeEnd:
                 break
 
-            changeStart = end + 1
+            start = end + 1
 
     changelist = changes.keys()
     changelist.sort()
diff --git a/t/t9818-git-p4-block.sh b/t/t9818-git-p4-block.sh
index 64510b7..8840a18 100755
--- a/t/t9818-git-p4-block.sh
+++ b/t/t9818-git-p4-block.sh
@@ -128,7 +128,7 @@ test_expect_success 'Create a repo with multiple depot paths' '
 	done
 '
 
-test_expect_failure 'Clone repo with multiple depot paths' '
+test_expect_success 'Clone repo with multiple depot paths' '
 	(
 		cd "$git" &&
 		git p4 clone --changes-block-size=4 //depot/pathA@all //depot/pathB@all \

-- 
2.6.2.474.g3eb3291
Luke Diamand· Dec 13, 2015, 20:19 UTC · re: Luke Diamand · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

Having just fixed this, I've now just spotted that Sam Hocevar's fix to reduce the number of P4 transactions also fixes it:

https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html
That seems like a cleaner fix.
Luke
On 13 December 2015 at 20:07, Luke Diamand <luke@diamand.org> wrote:
Show 21 quoted lines
> James Farwell reported a bug I introduced into git-p4 with
> handling of multiple depot paths:
>
> http://article.gmane.org/gmane.comp.version-control.git/282297
>
> This patch series adds a failing test case, and a fix for this
> problem.
>
> Luke
>
> Luke Diamand (2):
>   git-p4: failing test case for skipping changes with multiple depots
>   git-p4: fix handling of multiple depot paths
>
>  git-p4.py               |  8 +++++---
>  t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-
>  2 files changed, 32 insertions(+), 4 deletions(-)
>
> --
> 2.6.2.474.g3eb3291
>
Junio C Hamano· Dec 14, 2015, 19:16 UTC · re: Luke Diamand · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

Luke Diamand <luke@diamand.org> writes:
Show 6 quoted lines
> Having just fixed this, I've now just spotted that Sam Hocevar's fix
> to reduce the number of P4 transactions also fixes it:
>
> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html
>
> That seems like a cleaner fix.

Hmm, do you mean I should ignore this series and take the other one, take only 1/2 from this for tests and then both patches in the other one, or something else?

Thanks.
Show 26 quoted lines
>
> Luke
>
>
> On 13 December 2015 at 20:07, Luke Diamand <luke@diamand.org> wrote:
>> James Farwell reported a bug I introduced into git-p4 with
>> handling of multiple depot paths:
>>
>> http://article.gmane.org/gmane.comp.version-control.git/282297
>>
>> This patch series adds a failing test case, and a fix for this
>> problem.
>>
>> Luke
>>
>> Luke Diamand (2):
>>   git-p4: failing test case for skipping changes with multiple depots
>>   git-p4: fix handling of multiple depot paths
>>
>>  git-p4.py               |  8 +++++---
>>  t/t9818-git-p4-block.sh | 28 +++++++++++++++++++++++++++-
>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>
>> --
>> 2.6.2.474.g3eb3291
>>
Luke Diamand· Dec 14, 2015, 20:58 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

On 14 December 2015 at 19:16, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> Luke Diamand <luke@diamand.org> writes:
>
>> Having just fixed this, I've now just spotted that Sam Hocevar's fix
>> to reduce the number of P4 transactions also fixes it:
>>
>> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html
>>
>> That seems like a cleaner fix.
>
> Hmm, do you mean I should ignore this series and take the other one,
> take only 1/2 from this for tests and then both patches in the other
> one, or something else?

The second of those (take only 1/2 from this for tests, and then both from the other) seems like the way to go.

Thanks, Luke

Junio C Hamano· Dec 14, 2015, 22:06 UTC · re: Luke Diamand · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

Luke Diamand <luke@diamand.org> writes:
Show 16 quoted lines
> On 14 December 2015 at 19:16, Junio C Hamano <gitster@pobox.com> wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix
>>> to reduce the number of P4 transactions also fixes it:
>>>
>>> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html
>>>
>>> That seems like a cleaner fix.
>>
>> Hmm, do you mean I should ignore this series and take the other one,
>> take only 1/2 from this for tests and then both patches in the other
>> one, or something else?
>
> The second of those (take only 1/2 from this for tests, and then both
> from the other) seems like the way to go.
OK.  Should I consider the two patches from Sam "Reviewed-by" you?
Luke Diamand· Dec 14, 2015, 23:09 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

Sorry - I've just run the tests, and this change causes one of the test cases in t9800-git-p4-basic.sh to fail.

It looks like the test case makes an assumption about who wins if two P4 depots have changes to files that end up in the same place, and this change reverses the order. It may actually be fine, but it needs to be thought about a bit.

Sam - do you have any thoughts on this?

Thanks Luke

On 14 December 2015 at 22:06, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> Luke Diamand <luke@diamand.org> writes:
>
>> On 14 December 2015 at 19:16, Junio C Hamano <gitster@pobox.com> wrote:
>>> Luke Diamand <luke@diamand.org> writes:
>>>
>>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix
>>>> to reduce the number of P4 transactions also fixes it:
>>>>
>>>> https://www.mail-archive.com/git%40vger.kernel.org/msg81880.html
>>>>
>>>> That seems like a cleaner fix.
>>>
>>> Hmm, do you mean I should ignore this series and take the other one,
>>> take only 1/2 from this for tests and then both patches in the other
>>> one, or something else?
>>
>> The second of those (take only 1/2 from this for tests, and then both
>> from the other) seems like the way to go.
>
> OK.  Should I consider the two patches from Sam "Reviewed-by" you?
James Farwell· Dec 15, 2015, 22:56 UTC · re: Luke Diamand · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

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. :)
- James
________________________________________
From: Luke Diamand <luke@diamand.org>
Sent: Monday, December 14, 2015 3:09 PM
To: Junio C Hamano
Cc: Git Users; James Farwell; Lars Schneider; Eric Sunshine; Sam Hocevar
Subject: Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

Sorry - I've just run the tests, and this change causes one of the test cases in t9800-git-p4-basic.sh to fail.

It looks like the test case makes an assumption about who wins if two P4 depots have changes to files that end up in the same place, and this change reverses the order. It may actually be fine, but it needs to be thought about a bit.

Sam - do you have any thoughts on this?

Thanks Luke

On 14 December 2015 at 22:06, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> Luke Diamand <luke@diamand.org> writes:
>
>> On 14 December 2015 at 19:16, Junio C Hamano <gitster@pobox.com> wrote:
>>> Luke Diamand <luke@diamand.org> writes:
>>>
>>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix
>>>> to reduce the number of P4 transactions also fixes it:
>>>>
>>>> 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=
>>>>
>>>> That seems like a cleaner fix.
>>>
>>> Hmm, do you mean I should ignore this series and take the other one,
>>> take only 1/2 from this for tests and then both patches in the other
>>> one, or something else?
>>
>> The second of those (take only 1/2 from this for tests, and then both
>> from the other) seems like the way to go.
>
> OK.  Should I consider the two patches from Sam "Reviewed-by" you?
Sam Hocevar· Dec 16, 2015, 00:38 UTC · re: James Farwell · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

   I'm actually surprised that the patch changes the order at all, since
all it does is affect the decision (on a yes/no basis) to include a given
file into a changelist. I'm going to have a look at that specific unit
test, but of course as a user I'd prefer if the default behaviour could
remain the same, unless it was actually a bug.
-- 
Sam.

On Tue, Dec 15, 2015, James Farwell wrote:
> 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. :)
> 
> - James
> 
> ________________________________________
> From: Luke Diamand <luke@diamand.org>
> Sent: Monday, December 14, 2015 3:09 PM
> To: Junio C Hamano
> Cc: Git Users; James Farwell; Lars Schneider; Eric Sunshine; Sam Hocevar
> Subject: Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths
> 
> Sorry - I've just run the tests, and this change causes one of the
> test cases in t9800-git-p4-basic.sh to fail.
> 
> It looks like the test case makes an assumption about who wins if two
> P4 depots have changes to files that end up in the same place, and
> this change reverses the order. It may actually be fine, but it needs
> to be thought about a bit.
> 
> Sam - do you have any thoughts on this?
> 
> Thanks
> Luke
> 
> 
> 
> 
> 
> 
> On 14 December 2015 at 22:06, Junio C Hamano <gitster@pobox.com> wrote:
> > Luke Diamand <luke@diamand.org> writes:
> >
> >> On 14 December 2015 at 19:16, Junio C Hamano <gitster@pobox.com> wrote:
> >>> Luke Diamand <luke@diamand.org> writes:
> >>>
> >>>> Having just fixed this, I've now just spotted that Sam Hocevar's fix
> >>>> to reduce the number of P4 transactions also fixes it:
> >>>>
> >>>> 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=
> >>>>
> >>>> That seems like a cleaner fix.
> >>>
> >>> Hmm, do you mean I should ignore this series and take the other one,
> >>> take only 1/2 from this for tests and then both patches in the other
> >>> one, or something else?
> >>
> >> The second of those (take only 1/2 from this for tests, and then both
> >> from the other) seems like the way to go.
> >
> > OK.  Should I consider the two patches from Sam "Reviewed-by" you?
echo "creationism" | tr -d "holy godly goal"
Luke Diamand· Dec 16, 2015, 07:51 UTC · re: Sam Hocevar · lore

Re: [PATCH 0/2] git-p4: fix for handling of multiple depot paths

On 16/12/15 00:38, Sam Hocevar wrote:
Show 7 quoted lines
> I'm actually surprised that the patch changes the order at all,
> since all it does is affect the decision (on a yes/no basis) to
> include a given file into a changelist. I'm going to have a look at
> that specific unit test, but of course as a user I'd prefer if the
> default behaviour could remain the same, unless it was actually a
> bug.
>
We ask for changes in
   //depot/sub1/...@1,6
   //depot/sub2/...@1,6'
which gives us [4, 6, 3, 5].

The old code used to sort this list but this change removes the sort. Maybe putting the sort back would fix it?

> 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.

In answer to James' question, the test checks that the most recent change wins (i.e. applied in order).

Luke

← back to recent threads