threads / discuss / 38993

[PATCHv2 0/4] git-p4: fix filetype detection on files opened exclusively

Subject: [PATCHv2 0/4] git-p4: fix filetype detection on files opened exclusively

## tl;dr

8 messages between Apr 3, 2015 and Apr 3, 2015.

replies: 7people: 3as markdown or json

Luke Diamand· Apr 3, 2015, 18:53 UTC · lore

This is a followup series to Blair's patch to fix filetype detection on files opened exclusively. It updates the git-p4 unit tests to catchup with that fix, fixing a couple of small bugs in the original tests.

Holloway, Blair (1):
  git-p4: fix filetype detection on files opened exclusively

Luke Diamand (3):
  git-p4: fix small bug in locked test scripts
  git-p4: small fix for locked-file-move-test
  git-p4: update locked test case

 git-p4.py                |  2 +-
 t/t9816-git-p4-locked.sh | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.3.0.rc1.30.g76afe74

Luke Diamand· Apr 3, 2015, 18:53 UTC · re: Luke Diamand · lore

[PATCHv2 1/4] git-p4: fix small bug in locked test scripts

Test script t9816-git-p4-locked.sh test #4 tests for adding a file that is locked by Perforce automatially. This is currently not supported by git-p4 and so is expected to fail.

However, a small typo meant it always failed, even with a fixed git-p4. Fix the typo to resolve this.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9816-git-p4-locked.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
index e71e543..ce0eb22 100755
--- a/t/t9816-git-p4-locked.sh
+++ b/t/t9816-git-p4-locked.sh
@@ -41,7 +41,7 @@ test_expect_failure 'add with lock not taken' '
 	(
 		cd "$git" &&
 		echo line1 >>add-lock-not-taken &&
-		git add file2 &&
+		git add add-lock-not-taken &&
 		git commit -m "add add-lock-not-taken" &&
 		git config git-p4.skipSubmitEdit true &&
 		git p4 submit --verbose

-- 
2.3.0.rc1.30.g76afe74

Eric Sunshine· Apr 3, 2015, 19:02 UTC · re: Luke Diamand · lore

Re: [PATCHv2 1/4] git-p4: fix small bug in locked test scripts

On Fri, Apr 3, 2015 at 2:53 PM, Luke Diamand <luke@diamand.org> wrote:

> Test script t9816-git-p4-locked.sh test #4 tests for
> adding a file that is locked by Perforce automatially.

s/automatially/automatically/

Show 26 quoted lines

> This is currently not supported by git-p4 and so is
> expected to fail.
>
> However, a small typo meant it always failed, even with
> a fixed git-p4. Fix the typo to resolve this.
>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
>  t/t9816-git-p4-locked.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
> index e71e543..ce0eb22 100755
> --- a/t/t9816-git-p4-locked.sh
> +++ b/t/t9816-git-p4-locked.sh
> @@ -41,7 +41,7 @@ test_expect_failure 'add with lock not taken' '
>         (
>                 cd "$git" &&
>                 echo line1 >>add-lock-not-taken &&
> -               git add file2 &&
> +               git add add-lock-not-taken &&
>                 git commit -m "add add-lock-not-taken" &&
>                 git config git-p4.skipSubmitEdit true &&
>                 git p4 submit --verbose
> --
> 2.3.0.rc1.30.g76afe74

Luke Diamand· Apr 3, 2015, 18:53 UTC · re: Luke Diamand · lore

[PATCHv2 2/4] git-p4: small fix for locked-file-move-test

The test for handling of failure when trying to move a file that is locked by another client was not quite correct - it failed early on because the target file in the move already existed.

The test now fails because git-p4 does not properly detect that p4 has rejected the move, and instead just crashes. At present, git-p4 has no support for detecting that a file has been locked and reporting it to the user, so this is the expected outcome.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9816-git-p4-locked.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
index ce0eb22..464f10b 100755
--- a/t/t9816-git-p4-locked.sh
+++ b/t/t9816-git-p4-locked.sh
@@ -130,8 +130,8 @@ test_expect_failure 'move with lock taken' '
 	git p4 clone --dest="$git" //depot &&
 	(
 		cd "$git" &&
-		git mv file1 file2 &&
-		git commit -m "mv file1 to file2" &&
+		git mv file1 file3 &&
+		git commit -m "mv file1 to file3" &&
 		git config git-p4.skipSubmitEdit true &&
 		git config git-p4.detectRenames true &&
 		git p4 submit --verbose

-- 
2.3.0.rc1.30.g76afe74

Luke Diamand· Apr 3, 2015, 18:53 UTC · re: Luke Diamand · lore

[PATCHv2 3/4] git-p4: fix filetype detection on files opened exclusively

From: "Holloway, Blair" <Blair_Holloway@playstation.sony.com>

If a Perforce server is configured to automatically set +l (exclusive lock) on add of certain file types, git p4 submit will fail during getP4OpenedType, as the regex doesn't expect the trailing '*exclusive*' from p4 opened:

//depot/file.png#1 - add default change (binary+l) *exclusive*

Signed-off-by: Blair Holloway <blair_holloway@playstation.sony.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index ff132b2..d43482a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -368,7 +368,7 @@ def getP4OpenedType(file):
     # Returns the perforce file type for the given file.
 
     result = p4_read_pipe(["opened", wildcard_encode(file)])
-    match = re.match(".*\((.+)\)\r?$", result)
+    match = re.match(".*\((.+)\)( \*exclusive\*)?\r?$", result)
     if match:
         return match.group(1)
     else:

-- 
2.3.0.rc1.30.g76afe74

Luke Diamand· Apr 3, 2015, 18:53 UTC · re: Luke Diamand · lore

[PATCHv2 4/4] git-p4: update locked test case

The add-new-file and copy-existing-file tests from t9816-git-p4-locked.sh now pass. Update the test case accordingly.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 t/t9816-git-p4-locked.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
index 464f10b..d048bd3 100755
--- a/t/t9816-git-p4-locked.sh
+++ b/t/t9816-git-p4-locked.sh
@@ -35,7 +35,7 @@ test_expect_success 'edit with lock not taken' '
 	)
 '
 
-test_expect_failure 'add with lock not taken' '
+test_expect_success 'add with lock not taken' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&
 	(
@@ -107,7 +107,7 @@ test_expect_failure 'chmod with lock taken' '
 	)
 '
 
-test_expect_failure 'copy with lock taken' '
+test_expect_success 'copy with lock taken' '
 	lock_in_another_client &&
 	test_when_finished cleanup_git &&
 	test_when_finished "cd \"$cli\" && p4 revert file2 && rm -f file2" &&

-- 
2.3.0.rc1.30.g76afe74

Eric Sunshine· Apr 3, 2015, 20:00 UTC · re: Luke Diamand · lore

Re: [PATCHv2 4/4] git-p4: update locked test case

On Fri, Apr 3, 2015 at 2:53 PM, Luke Diamand <luke@diamand.org> wrote:

> The add-new-file and copy-existing-file tests from
> t9816-git-p4-locked.sh now pass. Update the test
> case accordingly.

This patch should be folded into patch 3/4, shouldn't it?

Show 29 quoted lines

> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
>  t/t9816-git-p4-locked.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
> index 464f10b..d048bd3 100755
> --- a/t/t9816-git-p4-locked.sh
> +++ b/t/t9816-git-p4-locked.sh
> @@ -35,7 +35,7 @@ test_expect_success 'edit with lock not taken' '
>         )
>  '
>
> -test_expect_failure 'add with lock not taken' '
> +test_expect_success 'add with lock not taken' '
>         test_when_finished cleanup_git &&
>         git p4 clone --dest="$git" //depot &&
>         (
> @@ -107,7 +107,7 @@ test_expect_failure 'chmod with lock taken' '
>         )
>  '
>
> -test_expect_failure 'copy with lock taken' '
> +test_expect_success 'copy with lock taken' '
>         lock_in_another_client &&
>         test_when_finished cleanup_git &&
>         test_when_finished "cd \"$cli\" && p4 revert file2 && rm -f file2" &&
> --
> 2.3.0.rc1.30.g76afe74

Junio C Hamano· Apr 3, 2015, 21:55 UTC · re: Eric Sunshine · lore

Re: [PATCHv2 4/4] git-p4: update locked test case

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 6 quoted lines

> On Fri, Apr 3, 2015 at 2:53 PM, Luke Diamand <luke@diamand.org> wrote:
>> The add-new-file and copy-existing-file tests from
>> t9816-git-p4-locked.sh now pass. Update the test
>> case accordingly.
>
> This patch should be folded into patch 3/4, shouldn't it?

If the fix and tests were done by the same person, we strongly prefer to see the code fix and test expectation update in the same commit.

What Luke is doing here is to spread the credits/blame to two people. If I were playing Luke's role, I might have squashed them into a single commit, kept the authorship of the whole thing under Blair Holloway's name, and extend the log message by mentioning that I flipped the test expectations, but I'd say either is fine for this change.

Show 29 quoted lines

>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>>  t/t9816-git-p4-locked.sh | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
>> index 464f10b..d048bd3 100755
>> --- a/t/t9816-git-p4-locked.sh
>> +++ b/t/t9816-git-p4-locked.sh
>> @@ -35,7 +35,7 @@ test_expect_success 'edit with lock not taken' '
>>         )
>>  '
>>
>> -test_expect_failure 'add with lock not taken' '
>> +test_expect_success 'add with lock not taken' '
>>         test_when_finished cleanup_git &&
>>         git p4 clone --dest="$git" //depot &&
>>         (
>> @@ -107,7 +107,7 @@ test_expect_failure 'chmod with lock taken' '
>>         )
>>  '
>>
>> -test_expect_failure 'copy with lock taken' '
>> +test_expect_success 'copy with lock taken' '
>>         lock_in_another_client &&
>>         test_when_finished cleanup_git &&
>>         test_when_finished "cd \"$cli\" && p4 revert file2 && rm -f file2" &&
>> --
>> 2.3.0.rc1.30.g76afe74