[PATCHv2] git-p4 worktree support

[PATCHv2] git-p4 worktree support

[Luke Diamand]

Second attempt at teaching git-p4 about worktrees.

Earlier discussion here:

http://marc.info/?l=git&m=148097985622294

Git-p4 exports GIT_DIR so that when it chdirs into the
P4 client area to apply the change, git commands called
from there will work correctly.

Luke Diamand (1):
  git-p4: support git worktrees

 git-p4.py               | 47 ++++++++++++++++++++++++++---------------------
 t/t9800-git-p4-basic.sh | 20 ++++++++++++++++++++
 2 files changed, 46 insertions(+), 21 deletions(-)

-- 
2.8.2.703.g78b384c.dirty

[PATCHv2] git-p4: support git worktrees

[Luke Diamand]

git-p4 would attempt to find the git directory using
its own specific code, which did not know about git
worktrees. This caused git operations to fail needlessly.

Rework it to use "git rev-parse --git-dir" instead, which
knows about worktrees.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py               | 47 ++++++++++++++++++++++++++---------------------
 t/t9800-git-p4-basic.sh | 20 ++++++++++++++++++++
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index fd5ca52..6aa8957 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -49,6 +49,13 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
 # Grab changes in blocks of this many revisions, unless otherwise requested
 defaultBlockSize = 512
 
+def gitdir():
+    d = read_pipe("git rev-parse --git-dir").strip()
+    if not d or len(d) == 0:
+        return None
+    else:
+        return d
+
 def p4_build_cmd(cmd):
     """Build a suitable p4 command line.
 
@@ -562,12 +569,6 @@ def currentGitBranch():
     else:
         return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
 
-def isValidGitDir(path):
-    if (os.path.exists(path + "/HEAD")
-        and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
-        return True;
-    return False
-
 def parseRevision(ref):
     return read_pipe("git rev-parse %s" % ref).strip()
 
@@ -3462,7 +3463,7 @@ class P4Sync(Command, P4UserMap):
         if self.tempBranches != []:
             for branch in self.tempBranches:
                 read_pipe("git update-ref -d %s" % branch)
-            os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))
+            os.rmdir(os.path.join(gitdir(), self.tempBranchLocation))
 
         # Create a symbolic ref p4/HEAD pointing to p4/<branch> to allow
         # a convenient shortcut refname "p4".
@@ -3678,23 +3679,27 @@ def main():
     (cmd, args) = parser.parse_args(sys.argv[2:], cmd);
     global verbose
     verbose = cmd.verbose
+
     if cmd.needsGit:
-        if cmd.gitdir == None:
-            cmd.gitdir = os.path.abspath(".git")
-            if not isValidGitDir(cmd.gitdir):
-                cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
-                if os.path.exists(cmd.gitdir):
-                    cdup = read_pipe("git rev-parse --show-cdup").strip()
-                    if len(cdup) > 0:
-                        chdir(cdup);
-
-        if not isValidGitDir(cmd.gitdir):
-            if isValidGitDir(cmd.gitdir + "/.git"):
-                cmd.gitdir += "/.git"
-            else:
+        if cmd.gitdir:
+            os.environ["GIT_DIR"] = cmd.gitdir
+
+            # did we get a valid git dir on the command line or via $GIT_DIR?
+            if not gitdir():
                 die("fatal: cannot locate git repository at %s" % cmd.gitdir)
 
-        os.environ["GIT_DIR"] = cmd.gitdir
+        else:
+            # already in a git directory?
+            if not gitdir():
+                die("fatal: not in a valid git repository")
+
+        cdup = read_pipe("git rev-parse --show-cdup").strip()
+        if len(cdup) > 0:
+            chdir(cdup);
+
+        # ensure subshells spawned in the p4 repo directory
+        # get the correct GIT_DIR
+        os.environ["GIT_DIR"] = os.path.abspath(gitdir())
 
     if not cmd.run(args):
         parser.print_help()
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 0730f18..093e9bd 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
 	)
 '
 
+test_expect_success 'submit from worktree' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$git" &&
+		git worktree add ../worktree-test
+	) &&
+	(
+		cd "$git/../worktree-test" &&
+		test_commit "worktree-commit" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit
+	) &&
+	(
+		cd "$cli" &&
+		p4 sync &&
+		test_path_is_file worktree-commit.t
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
2.8.2.703.g78b384c.dirty

Re: [PATCHv2] git-p4: support git worktrees

[Luke Diamand]

On 10 December 2016 at 21:57, Luke Diamand <luke@diamand.org> wrote:
> git-p4 would attempt to find the git directory using
> its own specific code, which did not know about git
> worktrees. This caused git operations to fail needlessly.
>
> Rework it to use "git rev-parse --git-dir" instead, which
> knows about worktrees.

Actually this doesn't work as well as the original version. "git
rev-parse --git-dir" won't go and find the ".git" subdirectory. The
previous version would go looking for it, so this would introduce a
regression.

Luke


>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
>  git-p4.py               | 47 ++++++++++++++++++++++++++---------------------
>  t/t9800-git-p4-basic.sh | 20 ++++++++++++++++++++
>  2 files changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..6aa8957 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -49,6 +49,13 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
>  # Grab changes in blocks of this many revisions, unless otherwise requested
>  defaultBlockSize = 512
>
> +def gitdir():
> +    d = read_pipe("git rev-parse --git-dir").strip()
> +    if not d or len(d) == 0:
> +        return None
> +    else:
> +        return d
> +
>  def p4_build_cmd(cmd):
>      """Build a suitable p4 command line.
>
> @@ -562,12 +569,6 @@ def currentGitBranch():
>      else:
>          return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
>
> -def isValidGitDir(path):
> -    if (os.path.exists(path + "/HEAD")
> -        and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
> -        return True;
> -    return False
> -
>  def parseRevision(ref):
>      return read_pipe("git rev-parse %s" % ref).strip()
>
> @@ -3462,7 +3463,7 @@ class P4Sync(Command, P4UserMap):
>          if self.tempBranches != []:
>              for branch in self.tempBranches:
>                  read_pipe("git update-ref -d %s" % branch)
> -            os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))
> +            os.rmdir(os.path.join(gitdir(), self.tempBranchLocation))
>
>          # Create a symbolic ref p4/HEAD pointing to p4/<branch> to allow
>          # a convenient shortcut refname "p4".
> @@ -3678,23 +3679,27 @@ def main():
>      (cmd, args) = parser.parse_args(sys.argv[2:], cmd);
>      global verbose
>      verbose = cmd.verbose
> +
>      if cmd.needsGit:
> -        if cmd.gitdir == None:
> -            cmd.gitdir = os.path.abspath(".git")
> -            if not isValidGitDir(cmd.gitdir):
> -                cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
> -                if os.path.exists(cmd.gitdir):
> -                    cdup = read_pipe("git rev-parse --show-cdup").strip()
> -                    if len(cdup) > 0:
> -                        chdir(cdup);
> -
> -        if not isValidGitDir(cmd.gitdir):
> -            if isValidGitDir(cmd.gitdir + "/.git"):
> -                cmd.gitdir += "/.git"
> -            else:
> +        if cmd.gitdir:
> +            os.environ["GIT_DIR"] = cmd.gitdir
> +
> +            # did we get a valid git dir on the command line or via $GIT_DIR?
> +            if not gitdir():
>                  die("fatal: cannot locate git repository at %s" % cmd.gitdir)
>
> -        os.environ["GIT_DIR"] = cmd.gitdir
> +        else:
> +            # already in a git directory?
> +            if not gitdir():
> +                die("fatal: not in a valid git repository")
> +
> +        cdup = read_pipe("git rev-parse --show-cdup").strip()
> +        if len(cdup) > 0:
> +            chdir(cdup);
> +
> +        # ensure subshells spawned in the p4 repo directory
> +        # get the correct GIT_DIR
> +        os.environ["GIT_DIR"] = os.path.abspath(gitdir())
>
>      if not cmd.run(args):
>          parser.print_help()
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 0730f18..093e9bd 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
>         )
>  '
>
> +test_expect_success 'submit from worktree' '
> +       test_when_finished cleanup_git &&
> +       git p4 clone --dest="$git" //depot &&
> +       (
> +               cd "$git" &&
> +               git worktree add ../worktree-test
> +       ) &&
> +       (
> +               cd "$git/../worktree-test" &&
> +               test_commit "worktree-commit" &&
> +               git config git-p4.skipSubmitEdit true &&
> +               git p4 submit
> +       ) &&
> +       (
> +               cd "$cli" &&
> +               p4 sync &&
> +               test_path_is_file worktree-commit.t
> +       )
> +'
> +
>  test_expect_success 'kill p4d' '
>         kill_p4d
>  '
> --
> 2.8.2.703.g78b384c.dirty
>

Re: [PATCHv2] git-p4: support git worktrees

[Duy Nguyen]

On Sun, Dec 11, 2016 at 2:19 PM, Luke Diamand <luke@diamand.org> wrote:
> On 10 December 2016 at 21:57, Luke Diamand <luke@diamand.org> wrote:
>> git-p4 would attempt to find the git directory using
>> its own specific code, which did not know about git
>> worktrees. This caused git operations to fail needlessly.
>>
>> Rework it to use "git rev-parse --git-dir" instead, which
>> knows about worktrees.
>
> Actually this doesn't work as well as the original version. "git
> rev-parse --git-dir" won't go and find the ".git" subdirectory. The
> previous version would go looking for it, so this would introduce a
> regression.

Maybe git rev-parse --git-path HEAD, then strip out the /HEAD part?
-- 
Duy