[PATCH 00/11] git p4 tests and a few bug fixes

[PATCH 00/11] git p4 tests and a few bug fixes

[Pete Wyckoff]

Most of this is work on tests for git p4.

Patch 03 is a regression fix, found and narrowed down thanks to
much work by Damien Gérard.  But it is obscure enough that I'm
not proposing it for a maintenance release.

There are a couple other behavior fixes, but again, these
are quite minor and can wait for the next release.

Pete Wyckoff (11):
  git p4 test: wildcards are supported
  git p4 test: ensure p4 symlink parsing works
  git p4: work around p4 bug that causes empty symlinks
  git p4 test: explicitly check p4 wildcard delete
  git p4 test: is_cli_file_writeable succeeds
  git p4 test: run as user "author"
  git p4 test: do not pollute /tmp
  git p4: handle files with wildcards when doing RCS scrubbing
  git p4: fix an error message when "p4 where" fails
  git p4 test: examine behavior with locked (+l) files
  git p4 doc: use two-line style for options with multiple spellings

 Documentation/git-p4.txt           |   6 +-
 git-p4.py                          |  17 +++--
 t/lib-git-p4.sh                    |  23 +++++-
 t/t9802-git-p4-filetype.sh         |  83 +++++++++++++++++++++
 t/t9805-git-p4-skip-submit-edit.sh |   6 +-
 t/t9807-git-p4-submit.sh           |   2 +-
 t/t9809-git-p4-client-view.sh      |  16 ++--
 t/t9812-git-p4-wildcards.sh        |  50 +++++++++++++
 t/t9813-git-p4-preserve-users.sh   |  38 ++++------
 t/t9816-git-p4-locked.sh           | 145 +++++++++++++++++++++++++++++++++++++
 10 files changed, 342 insertions(+), 44 deletions(-)
 create mode 100755 t/t9816-git-p4-locked.sh

-- 
1.8.5.2.320.g99957e5

[PATCH 01/11] git p4 test: wildcards are supported

[Pete Wyckoff]

Since 9d57c4a (git p4: implement view spec wildcards with "p4
where", 2013-08-30), all the wildcard types should be supported.
Change must-fail tests to mark that they now pass.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9809-git-p4-client-view.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 77f6349..23a827f 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -76,28 +76,28 @@ test_expect_success 'init depot' '
 '
 
 # double % for printf
-test_expect_success 'unsupported view wildcard %%n' '
+test_expect_success 'view wildcard %%n' '
 	client_view "//depot/%%%%1/sub/... //client/sub/%%%%1/..." &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
-test_expect_success 'unsupported view wildcard *' '
+test_expect_success 'view wildcard *' '
 	client_view "//depot/*/bar/... //client/*/bar/..." &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
-test_expect_success 'wildcard ... only supported at end of spec 1' '
+test_expect_success 'wildcard ... in the middle' '
 	client_view "//depot/.../file11 //client/.../file11" &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
-test_expect_success 'wildcard ... only supported at end of spec 2' '
+test_expect_success 'wildcard ... in the middle and at the end' '
 	client_view "//depot/.../a/... //client/.../a/..." &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
 test_expect_success 'basic map' '
-- 
1.8.5.2.320.g99957e5

[PATCH 02/11] git p4 test: ensure p4 symlink parsing works

[Pete Wyckoff]

While this happens to work, there was no test to make sure
that the basic importing of a symlink from p4 to git functioned.

Add a simple test to create a symlink in p4 and import it into git,
then verify that the symlink exists and has the correct target.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9802-git-p4-filetype.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index a82744b..94d7be9 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -250,6 +250,23 @@ test_expect_success 'ignore apple' '
 	)
 '
 
+test_expect_success SYMLINKS 'create p4 symlink' '
+	cd "$cli" &&
+	ln -s symlink-target symlink &&
+	p4 add symlink &&
+	p4 submit -d "add symlink"
+'
+
+test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		test -L symlink &&
+		test $(readlink symlink) = symlink-target
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.320.g99957e5

[PATCH 03/11] git p4: work around p4 bug that causes empty symlinks

[Pete Wyckoff]

Damien Gérard highlights an interesting problem.  Some p4
repositories end up with symlinks that have an empty target.  It
is not possible to create this with current p4, but they do
indeed exist.

The effect in git p4 is that "p4 print" on the symlink returns an
empty string, confusing the curret symlink-handling code.

Such broken repositories cause problems in p4 as well, even with
no git involved.  In p4, syncing to a change that includes a
bogus symlink causes errors:

    //depot/empty-symlink - updating /home/me/p4/empty-symlink
    rename: /home/me/p4/empty-symlink: No such file or directory

and leaves no symlink.

In git, replicate the p4 behavior by ignoring these bad symlinks.
If, in a later p4 revision, the symlink happens to point to
something non-null, the symlink will be replaced properly.

Add a big test for all this too.

This happens to be a regression introduced by 1292df1 (git-p4:
Fix occasional truncation of symlink contents., 2013-08-08) and
appeared first in 1.8.5.  But it only shows up only in p4
repositories of dubious character, so can wait for a proper
release.

Tested-by: Damien Gérard <damien@iwi.me>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                  |  9 ++++++-
 t/t9802-git-p4-filetype.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 5ea8bb8..e798ecf 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2075,7 +2075,14 @@ class P4Sync(Command, P4UserMap):
             # p4 print on a symlink sometimes contains "target\n";
             # if it does, remove the newline
             data = ''.join(contents)
-            if data[-1] == '\n':
+            if not data:
+                # Some version of p4 allowed creating a symlink that pointed
+                # to nothing.  This causes p4 errors when checking out such
+                # a change, and errors here too.  Work around it by ignoring
+                # the bad symlink; hopefully a future change fixes it.
+                print "\nIgnoring empty symlink in %s" % file['depotFile']
+                return
+            elif data[-1] == '\n':
                 contents = [data[:-1]]
             else:
                 contents = [data]
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 94d7be9..66d3fc9 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -267,6 +267,72 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
 	)
 '
 
+test_expect_success SYMLINKS 'empty symlink target' '
+	(
+		# first create the file as a file
+		cd "$cli" &&
+		>empty-symlink &&
+		p4 add empty-symlink &&
+		p4 submit -d "add empty-symlink as a file"
+	) &&
+	(
+		# now change it to be a symlink to "target1"
+		cd "$cli" &&
+		p4 edit empty-symlink &&
+		p4 reopen -t symlink empty-symlink &&
+		rm empty-symlink &&
+		ln -s target1 empty-symlink &&
+		p4 add empty-symlink &&
+		p4 submit -d "make empty-symlink point to target1"
+	) &&
+	(
+		# Hack the p4 depot to make the symlink point to nothing;
+		# this should not happen in reality, but shows up
+		# in p4 repos in the wild.
+		#
+		# The sed expression changes this:
+		#     @@
+		#     text
+		#     @target1
+		#     @
+		# to this:
+		#     @@
+		#     text
+		#     @@
+		#
+		cd "$db/depot" &&
+		sed "/@target1/{; s/target1/@/; n; d; }" \
+		    empty-symlink,v >empty-symlink,v.tmp &&
+		mv empty-symlink,v.tmp empty-symlink,v
+	) &&
+	(
+		# Make sure symlink really is empty.  Asking
+		# p4 to sync here will make it generate errors.
+		cd "$cli" &&
+		p4 print -q //depot/empty-symlink#2 >out &&
+		test ! -s out
+	) &&
+	test_when_finished cleanup_git &&
+
+	# make sure git p4 handles it without error
+	git p4 clone --dest="$git" //depot@all &&
+
+	# fix the symlink, make it point to "target2"
+	(
+		cd "$cli" &&
+		p4 open empty-symlink &&
+		rm empty-symlink &&
+		ln -s target2 empty-symlink &&
+		p4 submit -d "make empty-symlink point to target2"
+	) &&
+	cleanup_git &&
+	git p4 clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		test $(readlink empty-symlink) = target2
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.320.g99957e5

[PATCH 04/11] git p4 test: explicitly check p4 wildcard delete

[Pete Wyckoff]

There was no test where p4 deleted a file with a wildcard
character.  Make sure git p4 applies the wildcard decoding
properly when importing a delete that includes a wildcard.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9812-git-p4-wildcards.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index 6763325..f2ddbc5 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -161,6 +161,33 @@ test_expect_success 'wildcard files submit back to p4, delete' '
 	)
 '
 
+test_expect_success 'p4 deleted a wildcard file' '
+	(
+		cd "$cli" &&
+		echo "wild delete test" >wild@delete &&
+		p4 add -f wild@delete &&
+		p4 submit -d "add wild@delete"
+	) &&
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		test_path_is_file wild@delete
+	) &&
+	(
+		cd "$cli" &&
+		# must use its encoded name
+		p4 delete wild%40delete &&
+		p4 submit -d "delete wild@delete"
+	) &&
+	(
+		cd "$git" &&
+		git p4 sync &&
+		git merge --ff-only p4/master &&
+		test_path_is_missing wild@delete
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.320.g99957e5

[PATCH 05/11] git p4 test: is_cli_file_writeable succeeds

[Pete Wyckoff]

Commit e9df0f9 (git p4: cygwin p4 client does not mark read-only,
2013-01-26) fixed a problem with "test -w" on cygwin, but mistakenly
marked the new test as failing.  Fix this.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9807-git-p4-submit.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 1fb7bc7..4caf36e 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -17,7 +17,7 @@ test_expect_success 'init depot' '
 	)
 '
 
-test_expect_failure 'is_cli_file_writeable function' '
+test_expect_success 'is_cli_file_writeable function' '
 	(
 		cd "$cli" &&
 		echo a >a &&
-- 
1.8.5.2.320.g99957e5

[PATCH 06/11] git p4 test: run as user "author"

[Pete Wyckoff]

The tests use author@example.com as the canonical submitter,
but he does not have an entry in the p4 users database.
This causes the generated change description to complain
that the git and p4 users disagree.  The complaint message
is still valid, just isn't useful in tests.  It was was
introduced in 848de9c (git-p4: warn if git authorship won't
be retained, 2011-05-13).

Fix t9813 to use @example.com instead of @localhost due to
change in p4_add_user().  Move the function into the git p4
test library so author can be added at initialization time.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/lib-git-p4.sh                  | 15 ++++++++++++++-
 t/t9813-git-p4-preserve-users.sh | 38 ++++++++++++++------------------------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index ccd918e..4ff2bb1 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -47,9 +47,10 @@ P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
 
 P4PORT=localhost:$P4DPORT
 P4CLIENT=client
+P4USER=author
 P4EDITOR=:
 unset P4CHARSET
-export P4PORT P4CLIENT P4EDITOR P4CHARSET
+export P4PORT P4CLIENT P4USER P4EDITOR P4CHARSET
 
 db="$TRASH_DIRECTORY/db"
 cli="$TRASH_DIRECTORY/cli"
@@ -96,12 +97,24 @@ start_p4d() {
 		return 1
 	fi
 
+	# build a p4 user so author@example.com has an entry
+	p4_add_user author
+
 	# build a client
 	client_view "//depot/... //client/..." &&
 
 	return 0
 }
 
+p4_add_user() {
+	name=$1 &&
+	p4 user -f -i <<-EOF
+	User: $name
+	Email: $name@example.com
+	FullName: Dr. $name
+	EOF
+}
+
 kill_p4d() {
 	pid=$(cat "$pidfile")
 	# it had better exist for the first kill
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index f2e85e5..166b840 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -19,16 +19,6 @@ test_expect_success 'create files' '
 	)
 '
 
-p4_add_user() {
-	name=$1 fullname=$2 &&
-	p4 user -f -i <<-EOF &&
-	User: $name
-	Email: $name@localhost
-	FullName: $fullname
-	EOF
-	p4 passwd -P secret $name
-}
-
 p4_grant_admin() {
 	name=$1 &&
 	{
@@ -51,8 +41,8 @@ make_change_by_user() {
 
 # Test username support, submitting as user 'alice'
 test_expect_success 'preserve users' '
-	p4_add_user alice Alice &&
-	p4_add_user bob Bob &&
+	p4_add_user alice &&
+	p4_add_user bob &&
 	p4_grant_admin alice &&
 	git p4 clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
@@ -60,8 +50,8 @@ test_expect_success 'preserve users' '
 		cd "$git" &&
 		echo "username: a change by alice" >>file1 &&
 		echo "username: a change by bob" >>file2 &&
-		git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
-		git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
+		git commit --author "Alice <alice@example.com>" -m "a change by alice" file1 &&
+		git commit --author "Bob <bob@example.com>" -m "a change by bob" file2 &&
 		git config git-p4.skipSubmitEditCheck true &&
 		P4EDITOR=touch P4USER=alice P4PASSWD=secret git p4 commit --preserve-user &&
 		p4_check_commit_author file1 alice &&
@@ -78,7 +68,7 @@ test_expect_success 'refuse to preserve users without perms' '
 		cd "$git" &&
 		git config git-p4.skipSubmitEditCheck true &&
 		echo "username-noperms: a change by alice" >>file1 &&
-		git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
+		git commit --author "Alice <alice@example.com>" -m "perms: a change by alice" file1 &&
 		P4EDITOR=touch P4USER=bob P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		test_must_fail git p4 commit --preserve-user &&
@@ -94,9 +84,9 @@ test_expect_success 'preserve user where author is unknown to p4' '
 		cd "$git" &&
 		git config git-p4.skipSubmitEditCheck true &&
 		echo "username-bob: a change by bob" >>file1 &&
-		git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" file1 &&
+		git commit --author "Bob <bob@example.com>" -m "preserve: a change by bob" file1 &&
 		echo "username-unknown: a change by charlie" >>file1 &&
-		git commit --author "Charlie <charlie@localhost>" -m "preserve: a change by charlie" file1 &&
+		git commit --author "Charlie <charlie@example.com>" -m "preserve: a change by charlie" file1 &&
 		P4EDITOR=touch P4USER=alice P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		test_must_fail git p4 commit --preserve-user &&
@@ -121,24 +111,24 @@ test_expect_success 'not preserving user with mixed authorship' '
 	(
 		cd "$git" &&
 		git config git-p4.skipSubmitEditCheck true &&
-		p4_add_user derek Derek &&
+		p4_add_user derek &&
 
-		make_change_by_user usernamefile3 Derek derek@localhost &&
+		make_change_by_user usernamefile3 Derek derek@example.com &&
 		P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		git p4 commit |\
-		grep "git author derek@localhost does not match" &&
+		grep "git author derek@example.com does not match" &&
 
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
+		make_change_by_user usernamefile3 Charlie charlie@example.com &&
 		git p4 commit |\
-		grep "git author charlie@localhost does not match" &&
+		grep "git author charlie@example.com does not match" &&
 
-		make_change_by_user usernamefile3 alice alice@localhost &&
+		make_change_by_user usernamefile3 alice alice@example.com &&
 		git p4 commit |\
 		test_must_fail grep "git author.*does not match" &&
 
 		git config git-p4.skipUserNameCheck true &&
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
+		make_change_by_user usernamefile3 Charlie charlie@example.com &&
 		git p4 commit |\
 		test_must_fail grep "git author.*does not match" &&
 
-- 
1.8.5.2.320.g99957e5

[PATCH 07/11] git p4 test: do not pollute /tmp

[Pete Wyckoff]

Generating the submit template for p4 uses tempfile.mkstemp(),
which by default puts files in /tmp.  For a test that fails,
possibly on purpose, this is not cleaned up.  Run with TMPDIR
pointing into the trash directory so the temp files go away
with the test results.

To do this required some other minor changes.  First, the editor
is launched using system(editor + " " + template_file), using
shell expansion to build the command string.  This doesn't work
if editor has a space in it.  And is generally unwise as it's
easy to fool the shell into doing extra work.  Exec the args
directly, without shell expansion.

Second, without shell expansion, the trick of "P4EDITOR=:" used
in the tests doesn't work.  Use a real command, true, as the
non-interactive editor for testing.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                          | 2 +-
 t/lib-git-p4.sh                    | 8 +++++++-
 t/t9805-git-p4-skip-submit-edit.sh | 6 ++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index e798ecf..a4414b5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1220,7 +1220,7 @@ class P4Submit(Command, P4UserMap):
             editor = os.environ.get("P4EDITOR")
         else:
             editor = read_pipe("git var GIT_EDITOR").strip()
-        system(editor + " " + template_file)
+        system([editor, template_file])
 
         # If the file was not saved, prompt to see if this patch should
         # be skipped.  But skip this verification step if configured so.
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 4ff2bb1..5aa8adc 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -48,7 +48,7 @@ P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
 P4PORT=localhost:$P4DPORT
 P4CLIENT=client
 P4USER=author
-P4EDITOR=:
+P4EDITOR=true
 unset P4CHARSET
 export P4PORT P4CLIENT P4USER P4EDITOR P4CHARSET
 
@@ -57,6 +57,12 @@ cli="$TRASH_DIRECTORY/cli"
 git="$TRASH_DIRECTORY/git"
 pidfile="$TRASH_DIRECTORY/p4d.pid"
 
+# git p4 submit generates a temp file, which will
+# not get cleaned up if the submission fails.  Don't
+# clutter up /tmp on the test machine.
+TMPDIR="$TRASH_DIRECTORY"
+export TMPDIR
+
 start_p4d() {
 	mkdir -p "$db" "$cli" "$git" &&
 	rm -f "$pidfile" &&
diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index ff2cc79..8931188 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh
@@ -17,7 +17,7 @@ test_expect_success 'init depot' '
 	)
 '
 
-# this works because EDITOR is set to :
+# this works because P4EDITOR is set to true
 test_expect_success 'no config, unedited, say yes' '
 	git p4 clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
@@ -90,7 +90,9 @@ test_expect_success 'no config, edited' '
 		cd "$git" &&
 		echo line >>file1 &&
 		git commit -a -m "change 5" &&
-		P4EDITOR="" EDITOR="\"$TRASH_DIRECTORY/ed.sh\"" git p4 submit &&
+		P4EDITOR="$TRASH_DIRECTORY/ed.sh" &&
+		export P4EDITOR &&
+		git p4 submit &&
 		p4 changes //depot/... >wc &&
 		test_line_count = 5 wc
 	)
-- 
1.8.5.2.320.g99957e5

[PATCH 08/11] git p4: handle files with wildcards when doing RCS scrubbing

[Pete Wyckoff]

Commit 9d7d446 (git p4: submit files with wildcards, 2012-04-29)
fixed problems with handling files that had p4 wildcard
characters, like "@" and "*".  But it missed one case, that of
RCS keyword scrubbing, which uses "p4 fstat" to extract type
information.  Fix it by calling wildcard_encode() on the raw
filename.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                   |  4 ++--
 t/t9812-git-p4-wildcards.sh | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index a4414b5..26b874f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -310,8 +310,8 @@ def split_p4_type(p4type):
 #
 # return the raw p4 type of a file (text, text+ko, etc)
 #
-def p4_type(file):
-    results = p4CmdList(["fstat", "-T", "headType", file])
+def p4_type(f):
+    results = p4CmdList(["fstat", "-T", "headType", wildcard_encode(f)])
     return results[0]['headType']
 
 #
diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index f2ddbc5..c7472cb 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -188,6 +188,29 @@ test_expect_success 'p4 deleted a wildcard file' '
 	)
 '
 
+test_expect_success 'wildcard files requiring keyword scrub' '
+	(
+		cd "$cli" &&
+		cat <<-\EOF >scrub@wild &&
+		$Id$
+		line2
+		EOF
+		p4 add -t text+k -f scrub@wild &&
+		p4 submit -d "scrub at wild"
+	) &&
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.attemptRCSCleanup true &&
+		sed "s/^line2/line2 edit/" <scrub@wild >scrub@wild.tmp &&
+		mv -f scrub@wild.tmp scrub@wild &&
+		git commit -m "scrub at wild line2 edit" scrub@wild &&
+		git p4 submit
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.320.g99957e5

[PATCH 09/11] git p4: fix an error message when "p4 where" fails

[Pete Wyckoff]

When "p4 where" fails, for whatever reason, the error message tries to
show an undefined variable.  This minor bug applies only when using a
client spec, and was introduced recently in 9d57c4a (git p4: implement
view spec wildcards with "p4 where", 2013-08-30).

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 26b874f..cdfa2df 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1871,7 +1871,7 @@ class View(object):
                 # assume error is "... file(s) not in client view"
                 continue
             if "clientFile" not in res:
-                die("No clientFile from 'p4 where %s'" % depot_path)
+                die("No clientFile in 'p4 where' output")
             if "unmap" in res:
                 # it will list all of them, but only one not unmap-ped
                 continue
-- 
1.8.5.2.320.g99957e5

[PATCH 10/11] git p4 test: examine behavior with locked (+l) files

[Pete Wyckoff]

The p4 server can enforce file locking, so that only one user
can edit a file at a time.  Git p4 is unable to submit changes
to locked files.  Currently it exits poorly.  Ideally it would
notice the locked condition and clean up nicely.

Add a bunch of tests that describe the problem, hoping that
fixes appear in the future.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9816-git-p4-locked.sh | 145 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)
 create mode 100755 t/t9816-git-p4-locked.sh

diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
new file mode 100755
index 0000000..e71e543
--- /dev/null
+++ b/t/t9816-git-p4-locked.sh
@@ -0,0 +1,145 @@
+#!/bin/sh
+
+test_description='git p4 locked file behavior'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+# See
+# http://www.perforce.com/perforce/doc.current/manuals/p4sag/03_superuser.html#1088563
+# for suggestions on how to configure "sitewide pessimistic locking"
+# where only one person can have a file open for edit at a time.
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		echo "TypeMap: +l //depot/..." | p4 typemap -i &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "add file1"
+	)
+'
+
+test_expect_success 'edit with lock not taken' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		echo line2 >>file1 &&
+		git add file1 &&
+		git commit -m "line2 in file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit
+	)
+'
+
+test_expect_failure 'add with lock not taken' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		echo line1 >>add-lock-not-taken &&
+		git add file2 &&
+		git commit -m "add add-lock-not-taken" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+lock_in_another_client() {
+	# build a different client
+	cli2="$TRASH_DIRECTORY/cli2" &&
+	mkdir -p "$cli2" &&
+	test_when_finished "p4 client -f -d client2 && rm -rf \"$cli2\"" &&
+	(
+		cd "$cli2" &&
+		P4CLIENT=client2 &&
+		cli="$cli2" &&
+		client_view "//depot/... //client2/..." &&
+		p4 sync &&
+		p4 open file1
+	)
+}
+
+test_expect_failure 'edit with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync -f file1" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		echo line3 >>file1 &&
+		git add file1 &&
+		git commit -m "line3 in file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'delete with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync -f file1" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git rm file1 &&
+		git commit -m "delete file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'chmod with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync -f file1" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		chmod +x file1 &&
+		git add file1 &&
+		git commit -m "chmod +x file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'copy with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 revert file2 && rm -f file2" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		cp file1 file2 &&
+		git add file2 &&
+		git commit -m "cp file1 to file2" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.detectCopies true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'move with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync file1 && rm -f file2" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git mv file1 file2 &&
+		git commit -m "mv file1 to file2" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.detectRenames true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.8.5.2.320.g99957e5

[PATCH 11/11] git p4 doc: use two-line style for options with multiple spellings

[Pete Wyckoff]

Thomas Rast noticed the docs have a mix of styles when
it comes to options with multiple spellings.  Standardize
the couple in git-p4.txt that are odd.

Instead of:
  -n, --dry-run::

Do this:
  -n::
  --dry-run::

See
http://thread.gmane.org/gmane.comp.version-control.git/219936/focus=219945

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 Documentation/git-p4.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 8cba16d..6ab5f94 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -168,7 +168,8 @@ All commands except clone accept these options.
 --git-dir <dir>::
 	Set the 'GIT_DIR' environment variable.  See linkgit:git[1].
 
---verbose, -v::
+-v::
+--verbose::
 	Provide more progress information.
 
 Sync options
@@ -279,7 +280,8 @@ These options can be used to modify 'git p4 submit' behavior.
 	Export tags from Git as p4 labels. Tags found in Git are applied
 	to the perforce working directory.
 
---dry-run, -n::
+-n::
+--dry-run::
 	Show just what commits would be submitted to p4; do not change
 	state in Git or p4.
 
-- 
1.8.5.2.320.g99957e5

Re: [PATCH 00/11] git p4 tests and a few bug fixes

[Junio C Hamano]

Pete Wyckoff <pw@padd.com> writes:

> Most of this is work on tests for git p4.
>
> Patch 03 is a regression fix, found and narrowed down thanks to
> much work by Damien Gérard.  But it is obscure enough that I'm
> not proposing it for a maintenance release.
>
> There are a couple other behavior fixes, but again, these
> are quite minor and can wait for the next release.

Thanks.

I am inclined to say that we should queue this on a fork from
'maint, merge the result to 'master' before 1.9-rc1 and ship the
result as part of the upcoming release, and then possibly merging
the topic to 1.8.5.x maintenance release after that.

This is primarily because I personally do not have p4 expertise to
test or properly judge this (iow, you are the area maintainer, the
authority), and I somehow have this feeling that parking in 'next'
for extended period of time would not give meaningfully larger
exposure to the code.

What do you think?

If you feel uneasy about such a fast-track, I wouldn't push it,
though.

> Pete Wyckoff (11):
>   git p4 test: wildcards are supported
>   git p4 test: ensure p4 symlink parsing works
>   git p4: work around p4 bug that causes empty symlinks
>   git p4 test: explicitly check p4 wildcard delete
>   git p4 test: is_cli_file_writeable succeeds
>   git p4 test: run as user "author"
>   git p4 test: do not pollute /tmp
>   git p4: handle files with wildcards when doing RCS scrubbing
>   git p4: fix an error message when "p4 where" fails
>   git p4 test: examine behavior with locked (+l) files
>   git p4 doc: use two-line style for options with multiple spellings
>
>  Documentation/git-p4.txt           |   6 +-
>  git-p4.py                          |  17 +++--
>  t/lib-git-p4.sh                    |  23 +++++-
>  t/t9802-git-p4-filetype.sh         |  83 +++++++++++++++++++++
>  t/t9805-git-p4-skip-submit-edit.sh |   6 +-
>  t/t9807-git-p4-submit.sh           |   2 +-
>  t/t9809-git-p4-client-view.sh      |  16 ++--
>  t/t9812-git-p4-wildcards.sh        |  50 +++++++++++++
>  t/t9813-git-p4-preserve-users.sh   |  38 ++++------
>  t/t9816-git-p4-locked.sh           | 145 +++++++++++++++++++++++++++++++++++++
>  10 files changed, 342 insertions(+), 44 deletions(-)
>  create mode 100755 t/t9816-git-p4-locked.sh

Re: [PATCH 03/11] git p4: work around p4 bug that causes empty symlinks

[Eric Sunshine]

On Tue, Jan 21, 2014 at 6:16 PM, Pete Wyckoff <pw@padd.com> wrote:
> Damien Gérard highlights an interesting problem.  Some p4
> repositories end up with symlinks that have an empty target.  It
> is not possible to create this with current p4, but they do
> indeed exist.
>
> The effect in git p4 is that "p4 print" on the symlink returns an
> empty string, confusing the curret symlink-handling code.
>
> Such broken repositories cause problems in p4 as well, even with
> no git involved.  In p4, syncing to a change that includes a
> bogus symlink causes errors:
>
>     //depot/empty-symlink - updating /home/me/p4/empty-symlink
>     rename: /home/me/p4/empty-symlink: No such file or directory
>
> and leaves no symlink.
>
> In git, replicate the p4 behavior by ignoring these bad symlinks.
> If, in a later p4 revision, the symlink happens to point to
> something non-null, the symlink will be replaced properly.
>
> Add a big test for all this too.
>
> This happens to be a regression introduced by 1292df1 (git-p4:
> Fix occasional truncation of symlink contents., 2013-08-08) and
> appeared first in 1.8.5.  But it only shows up only in p4

Redundant "only".

> repositories of dubious character, so can wait for a proper
> release.
>
> Tested-by: Damien Gérard <damien@iwi.me>
> Signed-off-by: Pete Wyckoff <pw@padd.com>

Re: [PATCH 06/11] git p4 test: run as user "author"

[Eric Sunshine]

On Tue, Jan 21, 2014 at 6:16 PM, Pete Wyckoff <pw@padd.com> wrote:
> The tests use author@example.com as the canonical submitter,
> but he does not have an entry in the p4 users database.
> This causes the generated change description to complain
> that the git and p4 users disagree.  The complaint message
> is still valid, just isn't useful in tests.  It was was

s/was was/was/

> introduced in 848de9c (git-p4: warn if git authorship won't
> be retained, 2011-05-13).
>
> Fix t9813 to use @example.com instead of @localhost due to
> change in p4_add_user().  Move the function into the git p4
> test library so author can be added at initialization time.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>

Re: [PATCH 00/11] git p4 tests and a few bug fixes

[Pete Wyckoff]

gitster@pobox.com wrote on Tue, 21 Jan 2014 16:03 -0800:
> Pete Wyckoff <pw@padd.com> writes:
[..]
> > Patch 03 is a regression fix, found and narrowed down thanks to
> > much work by Damien Gérard.  But it is obscure enough that I'm
> > not proposing it for a maintenance release.
> 
> Thanks.
> 
> I am inclined to say that we should queue this on a fork from
> 'maint, merge the result to 'master' before 1.9-rc1 and ship the
> result as part of the upcoming release, and then possibly merging
> the topic to 1.8.5.x maintenance release after that.
> 
> This is primarily because I personally do not have p4 expertise to
> test or properly judge this (iow, you are the area maintainer, the
> authority), and I somehow have this feeling that parking in 'next'
> for extended period of time would not give meaningfully larger
> exposure to the code.
> 
> What do you think?
> 
> If you feel uneasy about such a fast-track, I wouldn't push it,
> though.

I think you're right that fast-track is the best choice, and low
risk.  The diffs came out identical, and it merges cleanly to
master, and passes all tests in both.

Thanks Eric for the commit message fixes too!

Here comes a v2 that is otherwise identical, but based on
origin/maint from a couple weeks ago.

		-- Pete

[PATCHv2 01/11] git p4 test: wildcards are supported

[Pete Wyckoff]

Since 9d57c4a (git p4: implement view spec wildcards with "p4
where", 2013-08-30), all the wildcard types should be supported.
Change must-fail tests to mark that they now pass.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9809-git-p4-client-view.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 77f6349..23a827f 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -76,28 +76,28 @@ test_expect_success 'init depot' '
 '
 
 # double % for printf
-test_expect_success 'unsupported view wildcard %%n' '
+test_expect_success 'view wildcard %%n' '
 	client_view "//depot/%%%%1/sub/... //client/sub/%%%%1/..." &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
-test_expect_success 'unsupported view wildcard *' '
+test_expect_success 'view wildcard *' '
 	client_view "//depot/*/bar/... //client/*/bar/..." &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
-test_expect_success 'wildcard ... only supported at end of spec 1' '
+test_expect_success 'wildcard ... in the middle' '
 	client_view "//depot/.../file11 //client/.../file11" &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
-test_expect_success 'wildcard ... only supported at end of spec 2' '
+test_expect_success 'wildcard ... in the middle and at the end' '
 	client_view "//depot/.../a/... //client/.../a/..." &&
 	test_when_finished cleanup_git &&
-	test_must_fail git p4 clone --use-client-spec --dest="$git" //depot
+	git p4 clone --use-client-spec --dest="$git" //depot
 '
 
 test_expect_success 'basic map' '
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 02/11] git p4 test: ensure p4 symlink parsing works

[Pete Wyckoff]

While this happens to work, there was no test to make sure
that the basic importing of a symlink from p4 to git functioned.

Add a simple test to create a symlink in p4 and import it into git,
then verify that the symlink exists and has the correct target.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9802-git-p4-filetype.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index a82744b..94d7be9 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -250,6 +250,23 @@ test_expect_success 'ignore apple' '
 	)
 '
 
+test_expect_success SYMLINKS 'create p4 symlink' '
+	cd "$cli" &&
+	ln -s symlink-target symlink &&
+	p4 add symlink &&
+	p4 submit -d "add symlink"
+'
+
+test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		test -L symlink &&
+		test $(readlink symlink) = symlink-target
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 03/11] git p4: work around p4 bug that causes empty symlinks

[Pete Wyckoff]

Damien Gérard highlights an interesting problem.  Some p4
repositories end up with symlinks that have an empty target.  It
is not possible to create this with current p4, but they do
indeed exist.

The effect in git p4 is that "p4 print" on the symlink returns an
empty string, confusing the curret symlink-handling code.

Such broken repositories cause problems in p4 as well, even with
no git involved.  In p4, syncing to a change that includes a
bogus symlink causes errors:

    //depot/empty-symlink - updating /home/me/p4/empty-symlink
    rename: /home/me/p4/empty-symlink: No such file or directory

and leaves no symlink.

In git, replicate the p4 behavior by ignoring these bad symlinks.
If, in a later p4 revision, the symlink happens to point to
something non-null, the symlink will be replaced properly.

Add a big test for all this too.

This happens to be a regression introduced by 1292df1 (git-p4:
Fix occasional truncation of symlink contents., 2013-08-08) and
appeared first in 1.8.5.  But it shows up only in p4
repositories of dubious character, so can wait for a proper
release.

Tested-by: Damien Gérard <damien@iwi.me>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                  |  9 ++++++-
 t/t9802-git-p4-filetype.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 06a3cc6..3a20d15 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2075,7 +2075,14 @@ class P4Sync(Command, P4UserMap):
             # p4 print on a symlink sometimes contains "target\n";
             # if it does, remove the newline
             data = ''.join(contents)
-            if data[-1] == '\n':
+            if not data:
+                # Some version of p4 allowed creating a symlink that pointed
+                # to nothing.  This causes p4 errors when checking out such
+                # a change, and errors here too.  Work around it by ignoring
+                # the bad symlink; hopefully a future change fixes it.
+                print "\nIgnoring empty symlink in %s" % file['depotFile']
+                return
+            elif data[-1] == '\n':
                 contents = [data[:-1]]
             else:
                 contents = [data]
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 94d7be9..66d3fc9 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -267,6 +267,72 @@ test_expect_success SYMLINKS 'ensure p4 symlink parsed correctly' '
 	)
 '
 
+test_expect_success SYMLINKS 'empty symlink target' '
+	(
+		# first create the file as a file
+		cd "$cli" &&
+		>empty-symlink &&
+		p4 add empty-symlink &&
+		p4 submit -d "add empty-symlink as a file"
+	) &&
+	(
+		# now change it to be a symlink to "target1"
+		cd "$cli" &&
+		p4 edit empty-symlink &&
+		p4 reopen -t symlink empty-symlink &&
+		rm empty-symlink &&
+		ln -s target1 empty-symlink &&
+		p4 add empty-symlink &&
+		p4 submit -d "make empty-symlink point to target1"
+	) &&
+	(
+		# Hack the p4 depot to make the symlink point to nothing;
+		# this should not happen in reality, but shows up
+		# in p4 repos in the wild.
+		#
+		# The sed expression changes this:
+		#     @@
+		#     text
+		#     @target1
+		#     @
+		# to this:
+		#     @@
+		#     text
+		#     @@
+		#
+		cd "$db/depot" &&
+		sed "/@target1/{; s/target1/@/; n; d; }" \
+		    empty-symlink,v >empty-symlink,v.tmp &&
+		mv empty-symlink,v.tmp empty-symlink,v
+	) &&
+	(
+		# Make sure symlink really is empty.  Asking
+		# p4 to sync here will make it generate errors.
+		cd "$cli" &&
+		p4 print -q //depot/empty-symlink#2 >out &&
+		test ! -s out
+	) &&
+	test_when_finished cleanup_git &&
+
+	# make sure git p4 handles it without error
+	git p4 clone --dest="$git" //depot@all &&
+
+	# fix the symlink, make it point to "target2"
+	(
+		cd "$cli" &&
+		p4 open empty-symlink &&
+		rm empty-symlink &&
+		ln -s target2 empty-symlink &&
+		p4 submit -d "make empty-symlink point to target2"
+	) &&
+	cleanup_git &&
+	git p4 clone --dest="$git" //depot@all &&
+	(
+		cd "$git" &&
+		test $(readlink empty-symlink) = target2
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 04/11] git p4 test: explicitly check p4 wildcard delete

[Pete Wyckoff]

There was no test where p4 deleted a file with a wildcard
character.  Make sure git p4 applies the wildcard decoding
properly when importing a delete that includes a wildcard.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9812-git-p4-wildcards.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index 6763325..f2ddbc5 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -161,6 +161,33 @@ test_expect_success 'wildcard files submit back to p4, delete' '
 	)
 '
 
+test_expect_success 'p4 deleted a wildcard file' '
+	(
+		cd "$cli" &&
+		echo "wild delete test" >wild@delete &&
+		p4 add -f wild@delete &&
+		p4 submit -d "add wild@delete"
+	) &&
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		test_path_is_file wild@delete
+	) &&
+	(
+		cd "$cli" &&
+		# must use its encoded name
+		p4 delete wild%40delete &&
+		p4 submit -d "delete wild@delete"
+	) &&
+	(
+		cd "$git" &&
+		git p4 sync &&
+		git merge --ff-only p4/master &&
+		test_path_is_missing wild@delete
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 05/11] git p4 test: is_cli_file_writeable succeeds

[Pete Wyckoff]

Commit e9df0f9 (git p4: cygwin p4 client does not mark read-only,
2013-01-26) fixed a problem with "test -w" on cygwin, but mistakenly
marked the new test as failing.  Fix this.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9807-git-p4-submit.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 1fb7bc7..4caf36e 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -17,7 +17,7 @@ test_expect_success 'init depot' '
 	)
 '
 
-test_expect_failure 'is_cli_file_writeable function' '
+test_expect_success 'is_cli_file_writeable function' '
 	(
 		cd "$cli" &&
 		echo a >a &&
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 06/11] git p4 test: run as user "author"

[Pete Wyckoff]

The tests use author@example.com as the canonical submitter, but
he does not have an entry in the p4 users database.  This causes
the generated change description to complain that the git and p4
users disagree.  The complaint message is still valid, just isn't
useful in tests.  It was introduced in 848de9c (git-p4: warn if
git authorship won't be retained, 2011-05-13).

Fix t9813 to use @example.com instead of @localhost due to change
in p4_add_user().  Move the function into the git p4 test library
so author can be added at initialization time.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/lib-git-p4.sh                  | 15 ++++++++++++++-
 t/t9813-git-p4-preserve-users.sh | 38 ++++++++++++++------------------------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index ccd918e..4ff2bb1 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -47,9 +47,10 @@ P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
 
 P4PORT=localhost:$P4DPORT
 P4CLIENT=client
+P4USER=author
 P4EDITOR=:
 unset P4CHARSET
-export P4PORT P4CLIENT P4EDITOR P4CHARSET
+export P4PORT P4CLIENT P4USER P4EDITOR P4CHARSET
 
 db="$TRASH_DIRECTORY/db"
 cli="$TRASH_DIRECTORY/cli"
@@ -96,12 +97,24 @@ start_p4d() {
 		return 1
 	fi
 
+	# build a p4 user so author@example.com has an entry
+	p4_add_user author
+
 	# build a client
 	client_view "//depot/... //client/..." &&
 
 	return 0
 }
 
+p4_add_user() {
+	name=$1 &&
+	p4 user -f -i <<-EOF
+	User: $name
+	Email: $name@example.com
+	FullName: Dr. $name
+	EOF
+}
+
 kill_p4d() {
 	pid=$(cat "$pidfile")
 	# it had better exist for the first kill
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index f2e85e5..166b840 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -19,16 +19,6 @@ test_expect_success 'create files' '
 	)
 '
 
-p4_add_user() {
-	name=$1 fullname=$2 &&
-	p4 user -f -i <<-EOF &&
-	User: $name
-	Email: $name@localhost
-	FullName: $fullname
-	EOF
-	p4 passwd -P secret $name
-}
-
 p4_grant_admin() {
 	name=$1 &&
 	{
@@ -51,8 +41,8 @@ make_change_by_user() {
 
 # Test username support, submitting as user 'alice'
 test_expect_success 'preserve users' '
-	p4_add_user alice Alice &&
-	p4_add_user bob Bob &&
+	p4_add_user alice &&
+	p4_add_user bob &&
 	p4_grant_admin alice &&
 	git p4 clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
@@ -60,8 +50,8 @@ test_expect_success 'preserve users' '
 		cd "$git" &&
 		echo "username: a change by alice" >>file1 &&
 		echo "username: a change by bob" >>file2 &&
-		git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
-		git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
+		git commit --author "Alice <alice@example.com>" -m "a change by alice" file1 &&
+		git commit --author "Bob <bob@example.com>" -m "a change by bob" file2 &&
 		git config git-p4.skipSubmitEditCheck true &&
 		P4EDITOR=touch P4USER=alice P4PASSWD=secret git p4 commit --preserve-user &&
 		p4_check_commit_author file1 alice &&
@@ -78,7 +68,7 @@ test_expect_success 'refuse to preserve users without perms' '
 		cd "$git" &&
 		git config git-p4.skipSubmitEditCheck true &&
 		echo "username-noperms: a change by alice" >>file1 &&
-		git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
+		git commit --author "Alice <alice@example.com>" -m "perms: a change by alice" file1 &&
 		P4EDITOR=touch P4USER=bob P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		test_must_fail git p4 commit --preserve-user &&
@@ -94,9 +84,9 @@ test_expect_success 'preserve user where author is unknown to p4' '
 		cd "$git" &&
 		git config git-p4.skipSubmitEditCheck true &&
 		echo "username-bob: a change by bob" >>file1 &&
-		git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" file1 &&
+		git commit --author "Bob <bob@example.com>" -m "preserve: a change by bob" file1 &&
 		echo "username-unknown: a change by charlie" >>file1 &&
-		git commit --author "Charlie <charlie@localhost>" -m "preserve: a change by charlie" file1 &&
+		git commit --author "Charlie <charlie@example.com>" -m "preserve: a change by charlie" file1 &&
 		P4EDITOR=touch P4USER=alice P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		test_must_fail git p4 commit --preserve-user &&
@@ -121,24 +111,24 @@ test_expect_success 'not preserving user with mixed authorship' '
 	(
 		cd "$git" &&
 		git config git-p4.skipSubmitEditCheck true &&
-		p4_add_user derek Derek &&
+		p4_add_user derek &&
 
-		make_change_by_user usernamefile3 Derek derek@localhost &&
+		make_change_by_user usernamefile3 Derek derek@example.com &&
 		P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
 		export P4EDITOR P4USER P4PASSWD &&
 		git p4 commit |\
-		grep "git author derek@localhost does not match" &&
+		grep "git author derek@example.com does not match" &&
 
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
+		make_change_by_user usernamefile3 Charlie charlie@example.com &&
 		git p4 commit |\
-		grep "git author charlie@localhost does not match" &&
+		grep "git author charlie@example.com does not match" &&
 
-		make_change_by_user usernamefile3 alice alice@localhost &&
+		make_change_by_user usernamefile3 alice alice@example.com &&
 		git p4 commit |\
 		test_must_fail grep "git author.*does not match" &&
 
 		git config git-p4.skipUserNameCheck true &&
-		make_change_by_user usernamefile3 Charlie charlie@localhost &&
+		make_change_by_user usernamefile3 Charlie charlie@example.com &&
 		git p4 commit |\
 		test_must_fail grep "git author.*does not match" &&
 
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 07/11] git p4 test: do not pollute /tmp

[Pete Wyckoff]

Generating the submit template for p4 uses tempfile.mkstemp(),
which by default puts files in /tmp.  For a test that fails,
possibly on purpose, this is not cleaned up.  Run with TMPDIR
pointing into the trash directory so the temp files go away
with the test results.

To do this required some other minor changes.  First, the editor
is launched using system(editor + " " + template_file), using
shell expansion to build the command string.  This doesn't work
if editor has a space in it.  And is generally unwise as it's
easy to fool the shell into doing extra work.  Exec the args
directly, without shell expansion.

Second, without shell expansion, the trick of "P4EDITOR=:" used
in the tests doesn't work.  Use a real command, true, as the
non-interactive editor for testing.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                          | 2 +-
 t/lib-git-p4.sh                    | 8 +++++++-
 t/t9805-git-p4-skip-submit-edit.sh | 6 ++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 3a20d15..f0a327d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1220,7 +1220,7 @@ class P4Submit(Command, P4UserMap):
             editor = os.environ.get("P4EDITOR")
         else:
             editor = read_pipe("git var GIT_EDITOR").strip()
-        system(editor + " " + template_file)
+        system([editor, template_file])
 
         # If the file was not saved, prompt to see if this patch should
         # be skipped.  But skip this verification step if configured so.
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 4ff2bb1..5aa8adc 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -48,7 +48,7 @@ P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
 P4PORT=localhost:$P4DPORT
 P4CLIENT=client
 P4USER=author
-P4EDITOR=:
+P4EDITOR=true
 unset P4CHARSET
 export P4PORT P4CLIENT P4USER P4EDITOR P4CHARSET
 
@@ -57,6 +57,12 @@ cli="$TRASH_DIRECTORY/cli"
 git="$TRASH_DIRECTORY/git"
 pidfile="$TRASH_DIRECTORY/p4d.pid"
 
+# git p4 submit generates a temp file, which will
+# not get cleaned up if the submission fails.  Don't
+# clutter up /tmp on the test machine.
+TMPDIR="$TRASH_DIRECTORY"
+export TMPDIR
+
 start_p4d() {
 	mkdir -p "$db" "$cli" "$git" &&
 	rm -f "$pidfile" &&
diff --git a/t/t9805-git-p4-skip-submit-edit.sh b/t/t9805-git-p4-skip-submit-edit.sh
index ff2cc79..8931188 100755
--- a/t/t9805-git-p4-skip-submit-edit.sh
+++ b/t/t9805-git-p4-skip-submit-edit.sh
@@ -17,7 +17,7 @@ test_expect_success 'init depot' '
 	)
 '
 
-# this works because EDITOR is set to :
+# this works because P4EDITOR is set to true
 test_expect_success 'no config, unedited, say yes' '
 	git p4 clone --dest="$git" //depot &&
 	test_when_finished cleanup_git &&
@@ -90,7 +90,9 @@ test_expect_success 'no config, edited' '
 		cd "$git" &&
 		echo line >>file1 &&
 		git commit -a -m "change 5" &&
-		P4EDITOR="" EDITOR="\"$TRASH_DIRECTORY/ed.sh\"" git p4 submit &&
+		P4EDITOR="$TRASH_DIRECTORY/ed.sh" &&
+		export P4EDITOR &&
+		git p4 submit &&
 		p4 changes //depot/... >wc &&
 		test_line_count = 5 wc
 	)
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 08/11] git p4: handle files with wildcards when doing RCS scrubbing

[Pete Wyckoff]

Commit 9d7d446 (git p4: submit files with wildcards, 2012-04-29)
fixed problems with handling files that had p4 wildcard
characters, like "@" and "*".  But it missed one case, that of
RCS keyword scrubbing, which uses "p4 fstat" to extract type
information.  Fix it by calling wildcard_encode() on the raw
filename.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                   |  4 ++--
 t/t9812-git-p4-wildcards.sh | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index f0a327d..39a0fa0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -310,8 +310,8 @@ def split_p4_type(p4type):
 #
 # return the raw p4 type of a file (text, text+ko, etc)
 #
-def p4_type(file):
-    results = p4CmdList(["fstat", "-T", "headType", file])
+def p4_type(f):
+    results = p4CmdList(["fstat", "-T", "headType", wildcard_encode(f)])
     return results[0]['headType']
 
 #
diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index f2ddbc5..c7472cb 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -188,6 +188,29 @@ test_expect_success 'p4 deleted a wildcard file' '
 	)
 '
 
+test_expect_success 'wildcard files requiring keyword scrub' '
+	(
+		cd "$cli" &&
+		cat <<-\EOF >scrub@wild &&
+		$Id$
+		line2
+		EOF
+		p4 add -t text+k -f scrub@wild &&
+		p4 submit -d "scrub at wild"
+	) &&
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.attemptRCSCleanup true &&
+		sed "s/^line2/line2 edit/" <scrub@wild >scrub@wild.tmp &&
+		mv -f scrub@wild.tmp scrub@wild &&
+		git commit -m "scrub at wild line2 edit" scrub@wild &&
+		git p4 submit
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 09/11] git p4: fix an error message when "p4 where" fails

[Pete Wyckoff]

When "p4 where" fails, for whatever reason, the error message tries to
show an undefined variable.  This minor bug applies only when using a
client spec, and was introduced recently in 9d57c4a (git p4: implement
view spec wildcards with "p4 where", 2013-08-30).

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 39a0fa0..db43629 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1871,7 +1871,7 @@ class View(object):
                 # assume error is "... file(s) not in client view"
                 continue
             if "clientFile" not in res:
-                die("No clientFile from 'p4 where %s'" % depot_path)
+                die("No clientFile in 'p4 where' output")
             if "unmap" in res:
                 # it will list all of them, but only one not unmap-ped
                 continue
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 10/11] git p4 test: examine behavior with locked (+l) files

[Pete Wyckoff]

The p4 server can enforce file locking, so that only one user
can edit a file at a time.  Git p4 is unable to submit changes
to locked files.  Currently it exits poorly.  Ideally it would
notice the locked condition and clean up nicely.

Add a bunch of tests that describe the problem, hoping that
fixes appear in the future.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9816-git-p4-locked.sh | 145 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)
 create mode 100755 t/t9816-git-p4-locked.sh

diff --git a/t/t9816-git-p4-locked.sh b/t/t9816-git-p4-locked.sh
new file mode 100755
index 0000000..e71e543
--- /dev/null
+++ b/t/t9816-git-p4-locked.sh
@@ -0,0 +1,145 @@
+#!/bin/sh
+
+test_description='git p4 locked file behavior'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+	start_p4d
+'
+
+# See
+# http://www.perforce.com/perforce/doc.current/manuals/p4sag/03_superuser.html#1088563
+# for suggestions on how to configure "sitewide pessimistic locking"
+# where only one person can have a file open for edit at a time.
+test_expect_success 'init depot' '
+	(
+		cd "$cli" &&
+		echo "TypeMap: +l //depot/..." | p4 typemap -i &&
+		echo file1 >file1 &&
+		p4 add file1 &&
+		p4 submit -d "add file1"
+	)
+'
+
+test_expect_success 'edit with lock not taken' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		echo line2 >>file1 &&
+		git add file1 &&
+		git commit -m "line2 in file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit
+	)
+'
+
+test_expect_failure 'add with lock not taken' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		echo line1 >>add-lock-not-taken &&
+		git add file2 &&
+		git commit -m "add add-lock-not-taken" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+lock_in_another_client() {
+	# build a different client
+	cli2="$TRASH_DIRECTORY/cli2" &&
+	mkdir -p "$cli2" &&
+	test_when_finished "p4 client -f -d client2 && rm -rf \"$cli2\"" &&
+	(
+		cd "$cli2" &&
+		P4CLIENT=client2 &&
+		cli="$cli2" &&
+		client_view "//depot/... //client2/..." &&
+		p4 sync &&
+		p4 open file1
+	)
+}
+
+test_expect_failure 'edit with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync -f file1" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		echo line3 >>file1 &&
+		git add file1 &&
+		git commit -m "line3 in file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'delete with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync -f file1" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git rm file1 &&
+		git commit -m "delete file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'chmod with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync -f file1" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		chmod +x file1 &&
+		git add file1 &&
+		git commit -m "chmod +x file1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'copy with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 revert file2 && rm -f file2" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		cp file1 file2 &&
+		git add file2 &&
+		git commit -m "cp file1 to file2" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.detectCopies true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_failure 'move with lock taken' '
+	lock_in_another_client &&
+	test_when_finished cleanup_git &&
+	test_when_finished "cd \"$cli\" && p4 sync file1 && rm -f file2" &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git mv file1 file2 &&
+		git commit -m "mv file1 to file2" &&
+		git config git-p4.skipSubmitEdit true &&
+		git config git-p4.detectRenames true &&
+		git p4 submit --verbose
+	)
+'
+
+test_expect_success 'kill p4d' '
+	kill_p4d
+'
+
+test_done
-- 
1.8.5.2.364.g6ac45cd

[PATCHv2 11/11] git p4 doc: use two-line style for options with multiple spellings

[Pete Wyckoff]

Thomas Rast noticed the docs have a mix of styles when
it comes to options with multiple spellings.  Standardize
the couple in git-p4.txt that are odd.

Instead of:
  -n, --dry-run::

Do this:
  -n::
  --dry-run::

See
http://thread.gmane.org/gmane.comp.version-control.git/219936/focus=219945

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 Documentation/git-p4.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 8cba16d..6ab5f94 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -168,7 +168,8 @@ All commands except clone accept these options.
 --git-dir <dir>::
 	Set the 'GIT_DIR' environment variable.  See linkgit:git[1].
 
---verbose, -v::
+-v::
+--verbose::
 	Provide more progress information.
 
 Sync options
@@ -279,7 +280,8 @@ These options can be used to modify 'git p4 submit' behavior.
 	Export tags from Git as p4 labels. Tags found in Git are applied
 	to the perforce working directory.
 
---dry-run, -n::
+-n::
+--dry-run::
 	Show just what commits would be submitted to p4; do not change
 	state in Git or p4.
 
-- 
1.8.5.2.364.g6ac45cd