[PATCH 0/8] git-p4 fixes and enhancements

[PATCH 0/8] git-p4 fixes and enhancements

[Pete Wyckoff]

I've collected a fair number of fixes and enhancements
to git-p4, and finally cleaned them up to send upstream.

      git-p4: test script
      git-p4: fix key error for p4 problem
      git-p4: add missing newline in initial import message
      git-p4: accommodate new move/delete type in p4
      git-p4: reinterpret confusing p4 message
      git-p4: better message for "git-p4 sync" when not cloned
      git-p4: decode p4 wildcard characters
      git-p4: support clone --bare

The first one is interesting in that it adds a test for
git-p4.  I'd appreciate a careful review of this in
particular.

The other seven fix problems and add small features, with
test cases where it makes sense.

		-- Pete

[PATCH 1/8] git-p4: test script

[Pete Wyckoff]

Add a basic test script for git-p4.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 t/t9800-git-p4.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)
 create mode 100755 t/t9800-git-p4.sh

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
new file mode 100755
index 0000000..f4956b7
--- /dev/null
+++ b/t/t9800-git-p4.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+
+test_description='git-p4 tests'
+
+. ./test-lib.sh
+
+p4 -h >/dev/null 2>&1
+retc=$?
+p4d -h >/dev/null 2>&1
+retd=$?
+if test $retc -ne 0 -o $retd -ne 0
+then
+	skip_all='skipping git-p4 tests; no p4 or p4d'
+	test_done
+fi
+
+GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4
+P4DPORT=10669
+
+db="$TRASH_DIRECTORY/db"
+cli="$TRASH_DIRECTORY/cli"
+git="$TRASH_DIRECTORY/git"
+
+test_debug 'echo p4d -q -d -r "$db" -p $P4DPORT'
+test_expect_success setup '
+	mkdir -p "$db" &&
+	p4d -q -d -r "$db" -p $P4DPORT &&
+	# wait for it to finish its initialization
+	sleep 1 &&
+	mkdir -p "$cli" &&
+	mkdir -p "$git" &&
+	export P4PORT=localhost:$P4DPORT
+'
+
+test_expect_success 'add p4 files' '
+	cd "$cli" &&
+	p4 client -i <<-EOF &&
+	Client: client
+	Description: client
+	Root: $cli
+	View: //depot/... //client/...
+	EOF
+	export P4CLIENT=client &&
+	echo file1 >file1 &&
+	p4 add file1 &&
+	p4 submit -d "file1" &&
+	cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'basic git-p4 clone' '
+	"$GITP4" clone --dest="$git" //depot &&
+	rm -rf "$git" && mkdir "$git"
+'
+
+test_expect_success 'shutdown' '
+	pid=`pgrep -f p4d` &&
+	test -n "$pid" &&
+	test_debug "ps wl `echo $pid`" &&
+	kill $pid
+'
+
+test_done
-- 
1.7.2.3

[PATCH 2/8] git-p4: fix key error for p4 problem

[Pete Wyckoff]

Some p4 failures result in an error, but the info['code'] is not
set.  These include a bad p4 executable, or a core dump from p4,
and other odd internal errors where p4 fails to generate proper
marshaled output.

Make sure the info key exists before using it to avoid a python
traceback.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |    5 ++++-
 t/t9800-git-p4.sh          |   13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 04ce7e3..2fefea4 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1440,10 +1440,13 @@ class P4Sync(Command):
                                            % (p, revision)
                                            for p in self.depotPaths])):
 
-            if info['code'] == 'error':
+            if 'code' in info and info['code'] == 'error':
                 sys.stderr.write("p4 returned an error: %s\n"
                                  % info['data'])
                 sys.exit(1)
+            if 'p4ExitCode' in info:
+                sys.stderr.write("p4 exitcode: %s\n" % info['p4ExitCode'])
+                sys.exit(1)
 
 
             change = int(info["change"])
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index f4956b7..41e57bb 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -52,6 +52,19 @@ test_expect_success 'basic git-p4 clone' '
 	rm -rf "$git" && mkdir "$git"
 '
 
+test_expect_success 'exit when p4 fails to produce marshaled output' '
+	badp4dir="$TRASH_DIRECTORY/badp4dir" &&
+	mkdir -p "$badp4dir" &&
+	cat >"$badp4dir"/p4 <<-EOF &&
+	#!$SHELL_PATH
+	exit 1
+	EOF
+	chmod 755 "$badp4dir"/p4 &&
+	PATH="$badp4dir:$PATH" "$GITP4" clone --dest="$git" //depot >errs 2>&1 ; retval=$? &&
+	test $retval -eq 1 &&
+	test_must_fail grep -q Traceback errs
+'
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.2.3

[PATCH 3/8] git-p4: add missing newline in initial import message

[Pete Wyckoff]

The commit message looks wrong without the newline.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2fefea4..d2ba215 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1429,7 +1429,7 @@ class P4Sync(Command):
         print "Doing initial import of %s from revision %s into %s" % (' '.join(self.depotPaths), revision, self.branch)
 
         details = { "user" : "git perforce import user", "time" : int(time.time()) }
-        details["desc"] = ("Initial import of %s from the state at revision %s"
+        details["desc"] = ("Initial import of %s from the state at revision %s\n"
                            % (' '.join(self.depotPaths), revision))
         details["change"] = revision
         newestRevision = 0
-- 
1.7.2.3

[PATCH 4/8] git-p4: accommodate new move/delete type in p4

[Pete Wyckoff]

Change 562d53f (2010-11-21) recognized the new move/delete type
for git-p4 sync, but it can also show up in an initial clone and
labels output.  Instead of replicating this in three places,
hoist the definition somewhere global.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index d2ba215..db19b17 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -834,6 +834,8 @@ class P4Submit(Command):
         return True
 
 class P4Sync(Command):
+    delete_actions = ( "delete", "move/delete", "purge" )
+
     def __init__(self):
         Command.__init__(self)
         self.options = [
@@ -1038,10 +1040,10 @@ class P4Sync(Command):
 
             if includeFile:
                 filesForCommit.append(f)
-                if f['action'] not in ('delete', 'move/delete', 'purge'):
-                    filesToRead.append(f)
-                else:
+                if f['action'] in self.delete_actions:
                     filesToDelete.append(f)
+                else:
+                    filesToRead.append(f)
 
         # deleted files...
         for f in filesToDelete:
@@ -1127,7 +1129,7 @@ class P4Sync(Command):
 
                 cleanedFiles = {}
                 for info in files:
-                    if info["action"] in ("delete", "purge"):
+                    if info["action"] in self.delete_actions:
                         continue
                     cleanedFiles[info["depotFile"]] = info["rev"]
 
@@ -1453,7 +1455,7 @@ class P4Sync(Command):
             if change > newestRevision:
                 newestRevision = change
 
-            if info["action"] in ("delete", "purge"):
+            if info["action"] in self.delete_actions:
                 # don't increase the file cnt, otherwise details["depotFile123"] will have gaps!
                 #fileCnt = fileCnt + 1
                 continue
-- 
1.7.2.3

[PATCH 5/8] git-p4: reinterpret confusing p4 message

[Pete Wyckoff]

Error output will look like this:

glom$ git p4 clone //deopt
Importing from //deopt into .
Reinitialized existing Git repository in /tmp/x/.git/
Doing initial import of //deopt from revision #head into refs/remotes/p4/master
p4 returned an error: //deopt/... - must refer to client glom.

This particular p4 error is misleading.
Perhaps the depot path was misspelled.
Depot path:  //deopt

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index db19b17..6b847c4 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1445,6 +1445,10 @@ class P4Sync(Command):
             if 'code' in info and info['code'] == 'error':
                 sys.stderr.write("p4 returned an error: %s\n"
                                  % info['data'])
+                if info['data'].find("must refer to client") >= 0:
+                    sys.stderr.write("This particular p4 error is misleading.\n")
+                    sys.stderr.write("Perhaps the depot path was misspelled.\n");
+                    sys.stderr.write("Depot path:  %s\n" % " ".join(self.depotPaths))
                 sys.exit(1)
             if 'p4ExitCode' in info:
                 sys.stderr.write("p4 exitcode: %s\n" % info['p4ExitCode'])
-- 
1.7.2.3

[PATCH 6/8] git-p4: better message for "git-p4 sync" when not cloned

[Pete Wyckoff]

A common error is to do "git-p4 sync" in a repository that
was not initialized by "git-p4 clone".  There will be no
p4 refs.  The error message in this case is a traceback
for an assertion, which is confusing.

Change it instead to explain the likely problem.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 6b847c4..04e6c3d 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1676,6 +1676,8 @@ class P4Sync(Command):
 
                 changes.sort()
             else:
+                if not self.p4BranchesInGit:
+                    die("No remote p4 branches.  Perhaps you never did \"git p4 clone\" in here.");
                 if self.verbose:
                     print "Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
                                                               self.changeRange)
-- 
1.7.2.3

[PATCH 7/8] git-p4: decode p4 wildcard characters

[Pete Wyckoff]

There are four wildcard characters in p4.  Files with these
characters can be added to p4 repos using the "-f" option.
They are stored in %xx notation, and when checked out, p4
converts them back to normal.

This patch does the same thing when importing into git,
converting the four special characters.  Without this change,
the files appear with literal %xx in their names.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   13 +++++++++++++
 t/t9800-git-p4.sh          |   22 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 04e6c3d..5b08cd6 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -884,6 +884,18 @@ class P4Sync(Command):
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
 
+    # The p4 wildcards are not allowed in filenames.  It complains
+    # if you try to add them, but you can override with "-f", in
+    # which case it translates them into %xx encoding.  Search for
+    # and fix just these four characters.  Do % last so it does
+    # not inadvertantly create new %-escapes.
+    def wildcard_decode(self, path):
+        path = path.replace("%23", "#") \
+                   .replace("%2A", "*") \
+                   .replace("%40", "@") \
+                   .replace("%25", "%")
+        return path
+
     def extractFilesFromCommit(self, commit):
         self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
                              for path in self.cloneExclude]
@@ -962,6 +974,7 @@ class P4Sync(Command):
 	    return
 
         relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
+        relPath = self.wildcard_decode(relPath)
         if verbose:
             sys.stderr.write("%s\n" % relPath)
 
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 41e57bb..72c38af 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -65,6 +65,28 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
 	test_must_fail grep -q Traceback errs
 '
 
+test_expect_success 'add p4 files with wildcards in the names' '
+	cd "$cli" &&
+	echo file-wild-hash >file-wild#hash &&
+	echo file-wild-star >file-wild\*star &&
+	echo file-wild-at >file-wild@at &&
+	echo file-wild-percent >file-wild%percent &&
+	p4 add -f file-wild* &&
+	p4 submit -d "file wildcards" &&
+	cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'wildcard files git-p4 clone' '
+	"$GITP4" clone --dest="$git" //depot &&
+	cd "$git" &&
+	test -f file-wild#hash &&
+	test -f file-wild\*star &&
+	test -f file-wild@at &&
+	test -f file-wild%percent &&
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" && mkdir "$git"
+'
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.2.3

[PATCH 8/8] git-p4: support clone --bare

[Pete Wyckoff]

Just like git clone --bare, build a .git directory but no
checked out files.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |   17 +++++++++++++----
 t/t9800-git-p4.sh          |   10 ++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 5b08cd6..efc5dce 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1771,10 +1771,13 @@ class P4Clone(P4Sync):
                                  help="where to leave result of the clone"),
             optparse.make_option("-/", dest="cloneExclude",
                                  action="append", type="string",
-                                 help="exclude depot path")
+                                 help="exclude depot path"),
+            optparse.make_option("--bare", dest="cloneBare",
+                                 action="store_true", default=False),
         ]
         self.cloneDestination = None
         self.needsGit = False
+        self.cloneBare = False
 
     # This is required for the "append" cloneExclude action
     def ensure_value(self, attr, value):
@@ -1814,11 +1817,16 @@ class P4Clone(P4Sync):
             self.cloneDestination = self.defaultDestination(args)
 
         print "Importing from %s into %s" % (', '.join(depotPaths), self.cloneDestination)
+
         if not os.path.exists(self.cloneDestination):
             os.makedirs(self.cloneDestination)
         chdir(self.cloneDestination)
-        system("git init")
-        self.gitdir = os.getcwd() + "/.git"
+
+        init_cmd = [ "git", "init" ]
+        if self.cloneBare:
+            init_cmd.append("--bare")
+        subprocess.check_call(init_cmd)
+
         if not P4Sync.run(self, depotPaths):
             return False
         if self.branch != "master":
@@ -1828,7 +1836,8 @@ class P4Clone(P4Sync):
                 masterbranch = "refs/heads/p4/master"
             if gitBranchExists(masterbranch):
                 system("git branch master %s" % masterbranch)
-                system("git checkout -f")
+                if not self.cloneBare:
+                    system("git checkout -f")
             else:
                 print "Could not detect main branch. No checkout/master branch created."
 
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 72c38af..1e7639b 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -87,6 +87,16 @@ test_expect_success 'wildcard files git-p4 clone' '
 	rm -rf "$git" && mkdir "$git"
 '
 
+test_expect_success 'clone bare' '
+	"$GITP4" clone --dest="$git" --bare //depot &&
+	cd "$git" &&
+	test ! -d .git &&
+	bare=`git config --get core.bare` &&
+	test "$bare" = true &&
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" && mkdir "$git"
+'
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.2.3

Re: [PATCH 1/8] git-p4: test script

[Vitor Antunes]

Hi Pete,

Kudos on this patch! I'm looking forward in having it accepted, so that I can
create some test cases for my latest patches ;)

Thanks,
Vitor Antunes

Re: [PATCH 1/8] git-p4: test script

[Junio C Hamano]

Pete Wyckoff <pw@padd.com> writes:

> Add a basic test script for git-p4.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>
> ---
>  t/t9800-git-p4.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 62 insertions(+), 0 deletions(-)
>  create mode 100755 t/t9800-git-p4.sh
>
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> new file mode 100755
> index 0000000..f4956b7
> --- /dev/null
> +++ b/t/t9800-git-p4.sh
> @@ -0,0 +1,62 @@
> +#!/bin/sh
> +
> +test_description='git-p4 tests'
> +
> +. ./test-lib.sh
> +
> +p4 -h >/dev/null 2>&1
> +retc=$?
> +p4d -h >/dev/null 2>&1
> +retd=$?
> +if test $retc -ne 0 -o $retd -ne 0
> +then

Use of two global variables with short names makes me feel "yeek!".

	(p4 -h && p4d -h) >/dev/null 2>/dev/null ||
	{
		...
                test_done
	}

> +GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4
> +P4DPORT=10669
> +
> +db="$TRASH_DIRECTORY/db"
> +cli="$TRASH_DIRECTORY/cli"
> +git="$TRASH_DIRECTORY/git"
> +
> +test_debug 'echo p4d -q -d -r "$db" -p $P4DPORT'
> +test_expect_success setup '
> +	mkdir -p "$db" &&
> +	p4d -q -d -r "$db" -p $P4DPORT &&
> +	# wait for it to finish its initialization
> +	sleep 1 &&

Is there a guarantee that "1" is sufficiently long for everybody?

Otherwise this will be a flaky test that sometimes passes and sometimes
doesn't, which we try to avoid.

If the answer is "empirically 1 second is sufficient for 99.9% of people",
then I would have to guess that it is 0.8 second too long for majority of
people, in which case I would like to see us try harder to make it both
reliable and efficient.

Isn't there a "noop" command a client can issue against a working server
that fails when the server is not ready (or waits until the server becomes
ready)?

Re: [PATCH 1/8] git-p4: test script

[Pete Wyckoff]

gitster@pobox.com wrote on Sun, 06 Feb 2011 18:22 -0800:
> Pete Wyckoff <pw@padd.com> writes:
[..]
> Use of two global variables with short names makes me feel "yeek!".
> 
> 	(p4 -h && p4d -h) >/dev/null 2>/dev/null ||
> 	{
> 		...
>                 test_done
> 	}

Much nicer.  Thanks.

> > +	p4d -q -d -r "$db" -p $P4DPORT &&
> > +	# wait for it to finish its initialization
> > +	sleep 1 &&
> 
> Is there a guarantee that "1" is sufficiently long for everybody?
> 
> Otherwise this will be a flaky test that sometimes passes and sometimes
> doesn't, which we try to avoid.
> 
> If the answer is "empirically 1 second is sufficient for 99.9% of people",
> then I would have to guess that it is 0.8 second too long for majority of
> people, in which case I would like to see us try harder to make it both
> reliable and efficient.
> 
> Isn't there a "noop" command a client can issue against a working server
> that fails when the server is not ready (or waits until the server becomes
> ready)?

There is a noop ("p4 info") that I can use to test.  But turns
out I was wrong in even needing to sleep or wait for the "info"
test to complete.  In trying to get it to race, I found that p4d
is well-behaved.  Strace confirms that it does bind/listen before
daemonizing.  So that sleep can be removed.

I'll wait a while in case other comments come in, then send the
updated series to you.

		-- Pete

Re: [PATCH 3/8] git-p4: add missing newline in initial import message

[Tor Arvid Lund]

On Sat, Feb 5, 2011 at 11:51 PM, Pete Wyckoff <pw@padd.com> wrote:
> The commit message looks wrong without the newline.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>

Acked-By: Tor Arvid Lund <torarvid@gmail.com>

> ---
>  contrib/fast-import/git-p4 |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 2fefea4..d2ba215 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -1429,7 +1429,7 @@ class P4Sync(Command):
>         print "Doing initial import of %s from revision %s into %s" % (' '.join(self.depotPaths), revision, self.branch)
>
>         details = { "user" : "git perforce import user", "time" : int(time.time()) }
> -        details["desc"] = ("Initial import of %s from the state at revision %s"
> +        details["desc"] = ("Initial import of %s from the state at revision %s\n"
>                            % (' '.join(self.depotPaths), revision))
>         details["change"] = revision
>         newestRevision = 0
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 4/8] git-p4: accommodate new move/delete type in p4

[Tor Arvid Lund]

On Sat, Feb 5, 2011 at 11:52 PM, Pete Wyckoff <pw@padd.com> wrote:
> Change 562d53f (2010-11-21) recognized the new move/delete type
> for git-p4 sync, but it can also show up in an initial clone and
> labels output.  Instead of replicating this in three places,
> hoist the definition somewhere global.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>

Acked-By: Tor Arvid Lund <torarvid@gmail.com>

> ---
>  contrib/fast-import/git-p4 |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index d2ba215..db19b17 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -834,6 +834,8 @@ class P4Submit(Command):
>         return True
>
>  class P4Sync(Command):
> +    delete_actions = ( "delete", "move/delete", "purge" )
> +
>     def __init__(self):
>         Command.__init__(self)
>         self.options = [
> @@ -1038,10 +1040,10 @@ class P4Sync(Command):
>
>             if includeFile:
>                 filesForCommit.append(f)
> -                if f['action'] not in ('delete', 'move/delete', 'purge'):
> -                    filesToRead.append(f)
> -                else:
> +                if f['action'] in self.delete_actions:
>                     filesToDelete.append(f)
> +                else:
> +                    filesToRead.append(f)
>
>         # deleted files...
>         for f in filesToDelete:
> @@ -1127,7 +1129,7 @@ class P4Sync(Command):
>
>                 cleanedFiles = {}
>                 for info in files:
> -                    if info["action"] in ("delete", "purge"):
> +                    if info["action"] in self.delete_actions:
>                         continue
>                     cleanedFiles[info["depotFile"]] = info["rev"]
>
> @@ -1453,7 +1455,7 @@ class P4Sync(Command):
>             if change > newestRevision:
>                 newestRevision = change
>
> -            if info["action"] in ("delete", "purge"):
> +            if info["action"] in self.delete_actions:
>                 # don't increase the file cnt, otherwise details["depotFile123"] will have gaps!
>                 #fileCnt = fileCnt + 1
>                 continue
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 6/8] git-p4: better message for "git-p4 sync" when not cloned

[Tor Arvid Lund]

On Sat, Feb 5, 2011 at 11:52 PM, Pete Wyckoff <pw@padd.com> wrote:
> A common error is to do "git-p4 sync" in a repository that
> was not initialized by "git-p4 clone".  There will be no
> p4 refs.  The error message in this case is a traceback
> for an assertion, which is confusing.
>
> Change it instead to explain the likely problem.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>

Acked-By: Tor Arvid Lund <torarvid@gmail.com>

> ---
>  contrib/fast-import/git-p4 |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 6b847c4..04e6c3d 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -1676,6 +1676,8 @@ class P4Sync(Command):
>
>                 changes.sort()
>             else:
> +                if not self.p4BranchesInGit:
> +                    die("No remote p4 branches.  Perhaps you never did \"git p4 clone\" in here.");
>                 if self.verbose:
>                     print "Getting p4 changes for %s...%s" % (', '.join(self.depotPaths),
>                                                               self.changeRange)
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 7/8] git-p4: decode p4 wildcard characters

[Tor Arvid Lund]

On Sat, Feb 5, 2011 at 11:52 PM, Pete Wyckoff <pw@padd.com> wrote:
> There are four wildcard characters in p4.  Files with these
> characters can be added to p4 repos using the "-f" option.
> They are stored in %xx notation, and when checked out, p4
> converts them back to normal.
>
> This patch does the same thing when importing into git,
> converting the four special characters.  Without this change,
> the files appear with literal %xx in their names.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>
> ---
>  contrib/fast-import/git-p4 |   13 +++++++++++++
>  t/t9800-git-p4.sh          |   22 ++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 04e6c3d..5b08cd6 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -884,6 +884,18 @@ class P4Sync(Command):
>         if gitConfig("git-p4.syncFromOrigin") == "false":
>             self.syncWithOrigin = False
>
> +    # The p4 wildcards are not allowed in filenames.  It complains
> +    # if you try to add them, but you can override with "-f", in
> +    # which case it translates them into %xx encoding.  Search for
> +    # and fix just these four characters.  Do % last so it does
> +    # not inadvertantly create new %-escapes.
> +    def wildcard_decode(self, path):
> +        path = path.replace("%23", "#") \
> +                   .replace("%2A", "*") \

This probably works fine on UNIX platforms, but the asterisk '*'
character is not allowed in windows filenames. I don't really know
what perforce does in that scenario. Does it make the most sense to
just keep the %2A in the filename if we are running on windows (??)

    -- Tor Arvid

> +                   .replace("%40", "@") \
> +                   .replace("%25", "%")
> +        return path
> +
>     def extractFilesFromCommit(self, commit):
>         self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
>                              for path in self.cloneExclude]
> @@ -962,6 +974,7 @@ class P4Sync(Command):
>            return
>
>         relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
> +        relPath = self.wildcard_decode(relPath)
>         if verbose:
>             sys.stderr.write("%s\n" % relPath)
>
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 41e57bb..72c38af 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -65,6 +65,28 @@ test_expect_success 'exit when p4 fails to produce marshaled output' '
>        test_must_fail grep -q Traceback errs
>  '
>
> +test_expect_success 'add p4 files with wildcards in the names' '
> +       cd "$cli" &&
> +       echo file-wild-hash >file-wild#hash &&
> +       echo file-wild-star >file-wild\*star &&
> +       echo file-wild-at >file-wild@at &&
> +       echo file-wild-percent >file-wild%percent &&
> +       p4 add -f file-wild* &&
> +       p4 submit -d "file wildcards" &&
> +       cd "$TRASH_DIRECTORY"
> +'
> +
> +test_expect_success 'wildcard files git-p4 clone' '
> +       "$GITP4" clone --dest="$git" //depot &&
> +       cd "$git" &&
> +       test -f file-wild#hash &&
> +       test -f file-wild\*star &&
> +       test -f file-wild@at &&
> +       test -f file-wild%percent &&
> +       cd "$TRASH_DIRECTORY" &&
> +       rm -rf "$git" && mkdir "$git"
> +'
> +
>  test_expect_success 'shutdown' '
>        pid=`pgrep -f p4d` &&
>        test -n "$pid" &&
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 8/8] git-p4: support clone --bare

[Tor Arvid Lund]

On Sat, Feb 5, 2011 at 11:52 PM, Pete Wyckoff <pw@padd.com> wrote:
> Just like git clone --bare, build a .git directory but no
> checked out files.
>
> Signed-off-by: Pete Wyckoff <pw@padd.com>

Acked-By: Tor Arvid Lund <torarvid@gmail.com>

> ---
>  contrib/fast-import/git-p4 |   17 +++++++++++++----
>  t/t9800-git-p4.sh          |   10 ++++++++++
>  2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 5b08cd6..efc5dce 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -1771,10 +1771,13 @@ class P4Clone(P4Sync):
>                                  help="where to leave result of the clone"),
>             optparse.make_option("-/", dest="cloneExclude",
>                                  action="append", type="string",
> -                                 help="exclude depot path")
> +                                 help="exclude depot path"),
> +            optparse.make_option("--bare", dest="cloneBare",
> +                                 action="store_true", default=False),
>         ]
>         self.cloneDestination = None
>         self.needsGit = False
> +        self.cloneBare = False
>
>     # This is required for the "append" cloneExclude action
>     def ensure_value(self, attr, value):
> @@ -1814,11 +1817,16 @@ class P4Clone(P4Sync):
>             self.cloneDestination = self.defaultDestination(args)
>
>         print "Importing from %s into %s" % (', '.join(depotPaths), self.cloneDestination)
> +
>         if not os.path.exists(self.cloneDestination):
>             os.makedirs(self.cloneDestination)
>         chdir(self.cloneDestination)
> -        system("git init")
> -        self.gitdir = os.getcwd() + "/.git"
> +
> +        init_cmd = [ "git", "init" ]
> +        if self.cloneBare:
> +            init_cmd.append("--bare")
> +        subprocess.check_call(init_cmd)
> +
>         if not P4Sync.run(self, depotPaths):
>             return False
>         if self.branch != "master":
> @@ -1828,7 +1836,8 @@ class P4Clone(P4Sync):
>                 masterbranch = "refs/heads/p4/master"
>             if gitBranchExists(masterbranch):
>                 system("git branch master %s" % masterbranch)
> -                system("git checkout -f")
> +                if not self.cloneBare:
> +                    system("git checkout -f")
>             else:
>                 print "Could not detect main branch. No checkout/master branch created."
>
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 72c38af..1e7639b 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -87,6 +87,16 @@ test_expect_success 'wildcard files git-p4 clone' '
>        rm -rf "$git" && mkdir "$git"
>  '
>
> +test_expect_success 'clone bare' '
> +       "$GITP4" clone --dest="$git" --bare //depot &&
> +       cd "$git" &&
> +       test ! -d .git &&
> +       bare=`git config --get core.bare` &&
> +       test "$bare" = true &&
> +       cd "$TRASH_DIRECTORY" &&
> +       rm -rf "$git" && mkdir "$git"
> +'
> +
>  test_expect_success 'shutdown' '
>        pid=`pgrep -f p4d` &&
>        test -n "$pid" &&
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 7/8] git-p4: decode p4 wildcard characters

[Pete Wyckoff]

torarvid@gmail.com wrote on Tue, 08 Feb 2011 10:09 +0100:
> On Sat, Feb 5, 2011 at 11:52 PM, Pete Wyckoff <pw@padd.com> wrote:
> > +    # The p4 wildcards are not allowed in filenames.  It complains
> > +    # if you try to add them, but you can override with "-f", in
> > +    # which case it translates them into %xx encoding.  Search for
> > +    # and fix just these four characters.  Do % last so it does
> > +    # not inadvertantly create new %-escapes.
> > +    def wildcard_decode(self, path):
> > +        path = path.replace("%23", "#") \
> > +                   .replace("%2A", "*") \
> 
> This probably works fine on UNIX platforms, but the asterisk '*'
> character is not allowed in windows filenames. I don't really know
> what perforce does in that scenario. Does it make the most sense to
> just keep the %2A in the filename if we are running on windows (??)

I changed it to do the "*" translation if not self.isWindows, so
%2A will remain in the filename.  Good that you noticed it.

Just for giggles, I found a windows VM to test perforce on.
Built two files with different wildcards on a unix box, then
pointed a windows client at it:

C:\DOCUME~1\ADMINI~1\DESKTOP>set P4PORT=192.168.2.1:1666

C:\DOCUME~1\ADMINI~1\DESKTOP>p4 files //depot/...
//depot/file%25percent#1 - add change 1 (binary)
//depot/file%2Astar#1 - add change 1 (binary)

C:\DOCUME~1\ADMINI~1\DESKTOP>p4 client
Client soulfree saved.

C:\DOCUME~1\ADMINI~1\DESKTOP>p4 sync
//depot/file%25percent#1 - added as c:\Documents and Settings\Administrator\Desktop\file%percent
//depot/file%2Astar#1 - added as c:\Documents and Settings\Administrator\Desktop\file*star
open for write: c:\Documents and Settings\Administrator\Desktop\file*star: The filename, directory name, or volume label syntax is incorrect.

And only the one file was synced to the windows client.  So "*" is not
well handled in perforce on windows anyway.

Docs are not helpful:

http://www.perforce.com/perforce/doc.current/manuals/cmdref/o.fspecs.html#1041962

For git, leaving a %2A in a filename is better than an error, I believe.

Thanks for the other acks.

		-- Pete