threads / patch / 30952

patch, 2 partsgit p4: use "move" command for renames

Subject: [PATCH 0/2] git p4: use "move" command for renames

## tl;dr

5 messages between Jul 4, 2012 and Jul 9, 2012. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Pete Wyckoff· Jul 4, 2012, 13:40 UTC · lore

Recent p4 supports a "move" command that records explicitly that a file was moved from one place to another. It can be changed a bit during the move, too. Use this feature, if it exists, when renames are detected.

Gary sent these patches months ago, and I've been sitting on them far too long. Was hoping to move some other work out first, but it was not ready.

These commits are on origin/next, as they depend on changes in pw/git-p4-tests that was merged into next on Jul 3. They do not conflict with the other series in flight, "notice Jobs: ...".

Gary Gibbons (2):
  git p4: refactor diffOpts calculation
  git p4: add support for 'p4 move' in P4Submit
 Documentation/git-p4.txt | 10 +++---
 git-p4.py                | 86 ++++++++++++++++++++++++++++++++----------------
 t/t9814-git-p4-rename.sh | 16 ++++-----
 3 files changed, 72 insertions(+), 40 deletions(-)
-- 
1.7.11.1.125.g4a65fea
Pete Wyckoff· Jul 4, 2012, 13:40 UTC · re: Pete Wyckoff · lore

[PATCH 1/2] git p4: refactor diffOpts calculation

From: Gary Gibbons <ggibbons@perforce.com>
P4Submit.applyCommit()

To avoid recalculating the same diffOpts for each commit, move it out of applyCommit() and into the top-level run(). Also fix a bug in that code which interpreted the value of detectRenames as a string rather than as a boolean.

[pw: fix documentation, rearrange code a bit]
Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 Documentation/git-p4.txt | 10 ++++++----
 git-p4.py                | 52 +++++++++++++++++++++++++++++-------------------
 2 files changed, 38 insertions(+), 24 deletions(-)
Show changes to 2 files +38 −24

Documentation/git-p4.txt, git-p4.py

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index fe1f49b..8228f33 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -255,7 +255,7 @@ These options can be used to modify 'git p4 submit' behavior.
 	p4.  By default, this is the most recent p4 commit reachable
 	from 'HEAD'.
 
--M[<n>]::
+-M::
 	Detect renames.  See linkgit:git-diff[1].  Renames will be
 	represented in p4 using explicit 'move' operations.  There
 	is no corresponding option to detect copies, but there are
@@ -465,13 +465,15 @@ git-p4.useClientSpec::
 Submit variables
 ~~~~~~~~~~~~~~~~
 git-p4.detectRenames::
-	Detect renames.  See linkgit:git-diff[1].
+	Detect renames.  See linkgit:git-diff[1].  This can be true,
+	false, or a score as expected by 'git diff -M'.
 
 git-p4.detectCopies::
-	Detect copies.  See linkgit:git-diff[1].
+	Detect copies.  See linkgit:git-diff[1].  This can be true,
+	false, or a score as expected by 'git diff -C'.
 
 git-p4.detectCopiesHarder::
-	Detect copies harder.  See linkgit:git-diff[1].
+	Detect copies harder.  See linkgit:git-diff[1].  A boolean.
 
 git-p4.preserveUser::
 	On submit, re-author changes to reflect the git author,
diff --git a/git-p4.py b/git-p4.py
index f895a24..5fe509f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1046,27 +1046,8 @@ class P4Submit(Command, P4UserMap):
 
         (p4User, gitEmail) = self.p4UserForCommit(id)
 
-        if not self.detectRenames:
-            # If not explicitly set check the config variable
-            self.detectRenames = gitConfig("git-p4.detectRenames")
-
-        if self.detectRenames.lower() == "false" or self.detectRenames == "":
-            diffOpts = ""
-        elif self.detectRenames.lower() == "true":
-            diffOpts = "-M"
-        else:
-            diffOpts = "-M%s" % self.detectRenames
-
-        detectCopies = gitConfig("git-p4.detectCopies")
-        if detectCopies.lower() == "true":
-            diffOpts += " -C"
-        elif detectCopies != "" and detectCopies.lower() != "false":
-            diffOpts += " -C%s" % detectCopies
 
-        if gitConfig("git-p4.detectCopiesHarder", "--bool") == "true":
-            diffOpts += " --find-copies-harder"
-
-        diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (diffOpts, id, id))
+        diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (self.diffOpts, id, id))
         filesToAdd = set()
         filesToDelete = set()
         editedFiles = set()
@@ -1433,6 +1414,37 @@ class P4Submit(Command, P4UserMap):
         if self.preserveUser:
             self.checkValidP4Users(commits)
 
+        #
+        # Build up a set of options to be passed to diff when
+        # submitting each commit to p4.
+        #
+        if self.detectRenames:
+            # command-line -M arg
+            self.diffOpts = "-M"
+        else:
+            # If not explicitly set check the config variable
+            detectRenames = gitConfig("git-p4.detectRenames")
+
+            if detectRenames.lower() == "false" or detectRenames == "":
+                self.diffOpts = ""
+            elif detectRenames.lower() == "true":
+                self.diffOpts = "-M"
+            else:
+                self.diffOpts = "-M%s" % detectRenames
+
+        # no command-line arg for -C or --find-copies-harder, just
+        # config variables
+        detectCopies = gitConfig("git-p4.detectCopies")
+        if detectCopies.lower() == "false" or detectCopies == "":
+            pass
+        elif detectCopies.lower() == "true":
+            self.diffOpts += " -C"
+        else:
+            self.diffOpts += " -C%s" % detectCopies
+
+        if gitConfig("git-p4.detectCopiesHarder", "--bool") == "true":
+            self.diffOpts += " --find-copies-harder"
+
         while len(commits) > 0:
             commit = commits[0]
             commits = commits[1:]
-- 
1.7.11.1.125.g4a65fea
Pete Wyckoff· Jul 4, 2012, 13:40 UTC · re: Pete Wyckoff · lore

[PATCH 2/2] git p4: add support for 'p4 move' in P4Submit

From: Gary Gibbons <ggibbons@perforce.com>

For -M option (detectRenames) in P4Submit, use 'p4 move' rather than 'p4 integrate'. Check Perforce server for exisitence of 'p4 move' and use it if present, otherwise revert to 'p4 integrate'.

[pw: wildcard-encode src/dest, add/update tests, tweak code]
Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 git-p4.py                | 34 ++++++++++++++++++++++++++--------
 t/t9814-git-p4-rename.sh | 16 ++++++++--------
 2 files changed, 34 insertions(+), 16 deletions(-)
Show changes to 2 files +34 −16

git-p4.py, t/t9814-git-p4-rename.sh

diff --git a/git-p4.py b/git-p4.py
index 5fe509f..b79e6f0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -120,6 +120,15 @@ def p4_read_pipe_lines(c):
     real_cmd = p4_build_cmd(c)
     return read_pipe_lines(real_cmd)
 
+def p4_has_command(cmd):
+    """Ask p4 for help on this command.  If it returns an error, the
+       command does not exist in this version of p4."""
+    real_cmd = p4_build_cmd(["help", cmd])
+    p = subprocess.Popen(real_cmd, stdout=subprocess.PIPE,
+                                   stderr=subprocess.PIPE)
+    p.communicate()
+    return p.returncode == 0
+
 def system(cmd):
     expand = isinstance(cmd,basestring)
     if verbose:
@@ -157,6 +166,9 @@ def p4_revert(f):
 def p4_reopen(type, f):
     p4_system(["reopen", "-t", type, wildcard_encode(f)])
 
+def p4_move(src, dest):
+    p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)])
+
 #
 # Canonicalize the p4 type and return a tuple of the
 # base type, plus any modifiers.  See "p4 help filetypes"
@@ -850,6 +862,7 @@ class P4Submit(Command, P4UserMap):
         self.preserveUser = gitConfig("git-p4.preserveUser").lower() == "true"
         self.isWindows = (platform.system() == "Windows")
         self.exportLabels = False
+        self.p4HasMoveCommand = p4_has_command("move") 
 
     def check(self):
         if len(p4CmdList("opened ...")) > 0:
@@ -1046,7 +1059,6 @@ class P4Submit(Command, P4UserMap):
 
         (p4User, gitEmail) = self.p4UserForCommit(id)
 
-
         diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (self.diffOpts, id, id))
         filesToAdd = set()
         filesToDelete = set()
@@ -1087,17 +1099,23 @@ class P4Submit(Command, P4UserMap):
                 editedFiles.add(dest)
             elif modifier == "R":
                 src, dest = diff['src'], diff['dst']
-                p4_integrate(src, dest)
-                if diff['src_sha1'] != diff['dst_sha1']:
-                    p4_edit(dest)
+                if self.p4HasMoveCommand:
+                    p4_edit(src)        # src must be open before move
+                    p4_move(src, dest)  # opens for (move/delete, move/add)
                 else:
-                    pureRenameCopy.add(dest)
+                    p4_integrate(src, dest)
+                    if diff['src_sha1'] != diff['dst_sha1']:
+                        p4_edit(dest)
+                    else:
+                        pureRenameCopy.add(dest)
                 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
-                    p4_edit(dest)
+                    if not self.p4HasMoveCommand:
+                        p4_edit(dest)   # with move: already open, writable
                     filesToChangeExecBit[dest] = diff['dst_mode']
-                os.unlink(dest)
+                if not self.p4HasMoveCommand:
+                    os.unlink(dest)
+                    filesToDelete.add(src)
                 editedFiles.add(dest)
-                filesToDelete.add(src)
             else:
                 die("unknown modifier %s for %s" % (modifier, path))
 
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 84fffb3..8be74b6 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -77,16 +77,16 @@ test_expect_success 'detect renames' '
 		git commit -a -m "Rename file1 to file4" &&
 		git diff-tree -r -M HEAD &&
 		git p4 submit &&
-		p4 filelog //depot/file4 &&
-		p4 filelog //depot/file4 | test_must_fail grep -q "branch from" &&
+		p4 filelog //depot/file4 | tee filelog &&
+		! grep -q " from //depot" filelog &&
 
 		git mv file4 file5 &&
 		git commit -a -m "Rename file4 to file5" &&
 		git diff-tree -r -M HEAD &&
 		git config git-p4.detectRenames true &&
 		git p4 submit &&
-		p4 filelog //depot/file5 &&
-		p4 filelog //depot/file5 | grep -q "branch from //depot/file4" &&
+		p4 filelog //depot/file5 | tee filelog &&
+		grep -q " from //depot/file4" filelog &&
 
 		git mv file5 file6 &&
 		echo update >>file6 &&
@@ -97,8 +97,8 @@ test_expect_success 'detect renames' '
 		test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
 		git config git-p4.detectRenames $(($level + 2)) &&
 		git p4 submit &&
-		p4 filelog //depot/file6 &&
-		p4 filelog //depot/file6 | test_must_fail grep -q "branch from" &&
+		p4 filelog //depot/file6 | tee filelog &&
+		! grep -q " from //depot" filelog &&
 
 		git mv file6 file7 &&
 		echo update >>file7 &&
@@ -109,8 +109,8 @@ test_expect_success 'detect renames' '
 		test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
 		git config git-p4.detectRenames $(($level - 2)) &&
 		git p4 submit &&
-		p4 filelog //depot/file7 &&
-		p4 filelog //depot/file7 | grep -q "branch from //depot/file6"
+		p4 filelog //depot/file7 | tee filelog &&
+		grep -q " from //depot/file6" filelog
 	)
 '
 
-- 
1.7.11.1.125.g4a65fea
Junio C Hamano· Jul 6, 2012, 06:28 UTC · re: Pete Wyckoff · lore

Re: [PATCH 2/2] git p4: add support for 'p4 move' in P4Submit

Pete Wyckoff <pw@padd.com> writes:
Show 12 quoted lines
> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> index 84fffb3..8be74b6 100755
> --- a/t/t9814-git-p4-rename.sh
> +++ b/t/t9814-git-p4-rename.sh
> @@ -77,16 +77,16 @@ test_expect_success 'detect renames' '
>  		git commit -a -m "Rename file1 to file4" &&
>  		git diff-tree -r -M HEAD &&
>  		git p4 submit &&
> -		p4 filelog //depot/file4 &&
> -		p4 filelog //depot/file4 | test_must_fail grep -q "branch from" &&
> +		p4 filelog //depot/file4 | tee filelog &&
> +		! grep -q " from //depot" filelog &&

I am not a huge fan of using "tee" in our test scripts, especially as it means piping output of another command whose output (and presumably the behaviour) we care about, hiding its exit status.

Fixing the incorrect use of piping to "test_must_fail grep" is a good change, but is there anything wrong to do the above like this?

	p4 filelog //depot/file4 >filelog &&
	! grep -q " from //depot" filelog &&
Pete Wyckoff· Jul 9, 2012, 10:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] git p4: add support for 'p4 move' in P4Submit

gitster@pobox.com wrote on Thu, 05 Jul 2012 23:28 -0700:
Show 24 quoted lines
> Pete Wyckoff <pw@padd.com> writes:
> 
> > diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> > index 84fffb3..8be74b6 100755
> > --- a/t/t9814-git-p4-rename.sh
> > +++ b/t/t9814-git-p4-rename.sh
> > @@ -77,16 +77,16 @@ test_expect_success 'detect renames' '
> >  		git commit -a -m "Rename file1 to file4" &&
> >  		git diff-tree -r -M HEAD &&
> >  		git p4 submit &&
> > -		p4 filelog //depot/file4 &&
> > -		p4 filelog //depot/file4 | test_must_fail grep -q "branch from" &&
> > +		p4 filelog //depot/file4 | tee filelog &&
> > +		! grep -q " from //depot" filelog &&
> 
> I am not a huge fan of using "tee" in our test scripts, especially
> as it means piping output of another command whose output (and
> presumably the behaviour) we care about, hiding its exit status.
> 
> Fixing the incorrect use of piping to "test_must_fail grep" is a
> good change, but is there anything wrong to do the above like this?
> 
> 	p4 filelog //depot/file4 >filelog &&
> 	! grep -q " from //depot" filelog &&

I'd started growing fond of "tee" as it shows all the output, and isolates the grep as a separate step. Much easier to see the bad output when a test fails.

I'll switch around to your approach, adding a "cat filelog" line for interesting cases.

		-- Pete

← back to recent threads