patch, 8 partsgit-p4 fixes and enhancements

> 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

> +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 &&

> Use of two global variables with short names makes me feel "yeek!".
> 
> 	(p4 -h && p4d -h) >/dev/null 2>/dev/null ||
> 	{
> 		...
>                 test_done
> 	}

> > +	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)?

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

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

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

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

> 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 (??)

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