[PATCHv1 0/3] git-p4: improved unshelving

[PATCHv1 0/3] git-p4: improved unshelving

[Luke Diamand]

This patch series teaches the git-p4 unshelve command to handle
intervening changes to the Perforce files.

At the moment if you try to unshelve a file, and that file has been
modified since the shelving, git-p4 refuses. That is so that it
doesn't end up generating a commit containing deltas from several P4
changes.

This gets to be more annoying as time goes on and the files you are
interested in get updated by other people.

However, what we can do is to create a parent commit matching the
state of the tree when the shelve happened, which then lets git-p4
create a git commit containing just the changes that are wanted.

It's still impossible to determine the true state of the complete
tree when the P4 shelve was created, since this information is not
recorded by Perforce. Manual intervention is required to fix that.

There are also a few other smaller fixes, the main one being
that it no longer unshelves into refs/remotes/p4/master/unshelved, but
instead into refs/remotes/p4-unshelved.

That's because the git-p4 branch detection gets confused by branches
appearing in refs/remotes/p4.


Luke Diamand (3):
  git-p4: do not fail in verbose mode for missing 'fileSize' key
  git-p4: unshelve into refs/remotes/p4-unshelved, not
    refs/remotes/p4/unshelved
  git-p4: fully support unshelving changelists

 Documentation/git-p4.txt | 10 ++---
 git-p4.py                | 90 +++++++++++++++++++++++-----------------
 t/t9832-unshelve.sh      | 75 ++++++++++++++++++++++++++-------
 3 files changed, 117 insertions(+), 58 deletions(-)

-- 
2.19.1.272.gf84b9b09d4

[PATCHv1 1/3] git-p4: do not fail in verbose mode for missing 'fileSize' key

[Luke Diamand]

If deleting or moving a file, sometimes P4 doesn't report the file size.

The code handles this just fine but some logging crashes. Stop this
happening.

There was some earlier discussion on the list about this:

https://public-inbox.org/git/xmqq1sqpp1vv.fsf@gitster.mtv.corp.google.com/

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 7fab255584..5701bad06a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2775,7 +2775,10 @@ def streamOneP4File(self, file, contents):
         relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
         relPath = self.encodeWithUTF8(relPath)
         if verbose:
-            size = int(self.stream_file['fileSize'])
+            if 'fileSize' in self.stream_file:
+                size = int(self.stream_file['fileSize'])
+            else:
+                size = 0 # deleted files don't get a fileSize apparently
             sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024))
             sys.stdout.flush()
 
-- 
2.19.1.272.gf84b9b09d4

[PATCHv1 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Luke Diamand]

The branch detection code looks for branches under refs/remotes/p4/...
and can end up getting confused if there are unshelved changes in
there as well. This happens in the function p4BranchesInGit().

Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 Documentation/git-p4.txt | 6 +++---
 git-p4.py                | 3 ++-
 t/t9832-unshelve.sh      | 6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 41780a5aa9..c7705ae6e7 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -174,7 +174,7 @@ $ git p4 submit --update-shelve 1234 --update-shelve 2345
 Unshelve
 ~~~~~~~~
 Unshelving will take a shelved P4 changelist, and produce the equivalent git commit
-in the branch refs/remotes/p4/unshelved/<changelist>.
+in the branch refs/remotes/p4-unshelved/<changelist>.
 
 The git commit is created relative to the current origin revision (HEAD by default).
 If the shelved changelist's parent revisions differ, git-p4 will refuse to unshelve;
@@ -182,13 +182,13 @@ you need to be unshelving onto an equivalent tree.
 
 The origin revision can be changed with the "--origin" option.
 
-If the target branch in refs/remotes/p4/unshelved already exists, the old one will
+If the target branch in refs/remotes/p4-unshelved already exists, the old one will
 be renamed.
 
 ----
 $ git p4 sync
 $ git p4 unshelve 12345
-$ git show refs/remotes/p4/unshelved/12345
+$ git show p4/unshelved/12345
 <submit more changes via p4 to the same files>
 $ git p4 unshelve 12345
 <refuses to unshelve until git is in sync with p4 again>
diff --git a/git-p4.py b/git-p4.py
index 5701bad06a..76c18a22e9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3956,7 +3956,8 @@ def __init__(self):
         ]
         self.verbose = False
         self.noCommit = False
-        self.destbranch = "refs/remotes/p4/unshelved"
+        self.destbranch = "refs/remotes/p4-unshelved"
+        self.origin = "p4/master"
 
     def renameBranch(self, branch_name):
         """ Rename the existing branch to branch_name.N
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 48ec7679b8..c3d15ceea8 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -54,8 +54,8 @@ EOF
 		cd "$git" &&
 		change=$(last_shelved_change) &&
 		git p4 unshelve $change &&
-		git show refs/remotes/p4/unshelved/$change | grep -q "Further description" &&
-		git cherry-pick refs/remotes/p4/unshelved/$change &&
+		git show refs/remotes/p4-unshelved/$change | grep -q "Further description" &&
+		git cherry-pick refs/remotes/p4-unshelved/$change &&
 		test_path_is_file file2 &&
 		test_cmp file1 "$cli"/file1 &&
 		test_cmp file2 "$cli"/file2 &&
@@ -88,7 +88,7 @@ EOF
 		cd "$git" &&
 		change=$(last_shelved_change) &&
 		git p4 unshelve $change &&
-		git diff refs/remotes/p4/unshelved/$change.0 refs/remotes/p4/unshelved/$change | grep -q file3
+		git diff refs/remotes/p4-unshelved/$change.0 refs/remotes/p4-unshelved/$change | grep -q file3
 	)
 '
 
-- 
2.19.1.272.gf84b9b09d4

[PATCHv1 3/3] git-p4: fully support unshelving changelists

[Luke Diamand]

The previous git-p4 unshelve support would check for changes
in Perforce to the files being unshelved since the original
shelve, and would complain if any were found.

This was to ensure that the user wouldn't end up with both the
shelved change delta, and some deltas from other changes in their
git commit.

e.g. given fileA:
      the
      quick
      brown
      fox

  change1: s/the/The/   <- p4 shelve this change
  change2: s/fox/Fox/   <- p4 submit this change
  git p4 unshelve 1     <- FAIL

This change teaches the P4Unshelve class to always create a parent
commit which matches the P4 tree (for the files being unshelved) at
the point prior to the P4 shelve being created (which is reported
in the p4 description for a shelved changelist).

That then means git-p4 can always create a git commit matching the
P4 shelve that was originally created, without any extra deltas.

The user might still need to use the --origin option though - there
is no way for git-p4 to work out the versions of all of the other
*unchanged* files in the shelve, since this information is not recorded
by Perforce.

Additionally this fixes handling of shelved 'move' operations.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 Documentation/git-p4.txt |  4 +-
 git-p4.py                | 84 +++++++++++++++++++++++-----------------
 t/t9832-unshelve.sh      | 69 ++++++++++++++++++++++++++-------
 3 files changed, 106 insertions(+), 51 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index c7705ae6e7..d8332e99c1 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -177,8 +177,8 @@ Unshelving will take a shelved P4 changelist, and produce the equivalent git com
 in the branch refs/remotes/p4-unshelved/<changelist>.
 
 The git commit is created relative to the current origin revision (HEAD by default).
-If the shelved changelist's parent revisions differ, git-p4 will refuse to unshelve;
-you need to be unshelving onto an equivalent tree.
+A parent commit is created based on the origin, and then the unshelve commit is
+created based on that.
 
 The origin revision can be changed with the "--origin" option.
 
diff --git a/git-p4.py b/git-p4.py
index 76c18a22e9..1998c3e141 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1306,6 +1306,9 @@ def processContent(self, git_mode, relPath, contents):
             return LargeFileSystem.processContent(self, git_mode, relPath, contents)
 
 class Command:
+    delete_actions = ( "delete", "move/delete", "purge" )
+    add_actions = ( "add", "move/add" )
+
     def __init__(self):
         self.usage = "usage: %prog [options]"
         self.needsGit = True
@@ -2524,7 +2527,6 @@ def map_in_client(self, depot_path):
         return ""
 
 class P4Sync(Command, P4UserMap):
-    delete_actions = ( "delete", "move/delete", "purge" )
 
     def __init__(self):
         Command.__init__(self)
@@ -2612,20 +2614,7 @@ def checkpoint(self):
         if self.verbose:
             print("checkpoint finished: " + out)
 
-    def cmp_shelved(self, path, filerev, revision):
-        """ Determine if a path at revision #filerev is the same as the file
-            at revision @revision for a shelved changelist. If they don't match,
-            unshelving won't be safe (we will get other changes mixed in).
-
-            This is comparing the revision that the shelved changelist is *based* on, not
-            the shelved changelist itself.
-        """
-        ret = p4Cmd(["diff2", "{0}#{1}".format(path, filerev), "{0}@{1}".format(path, revision)])
-        if verbose:
-            print("p4 diff2 path %s filerev %s revision %s => %s" % (path, filerev, revision, ret))
-        return ret["status"] == "identical"
-
-    def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0, origin_revision = 0):
+    def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
         self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
                              for path in self.cloneExclude]
         files = []
@@ -2650,17 +2639,6 @@ def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0, origin_r
             file["type"] = commit["type%s" % fnum]
             if shelved:
                 file["shelved_cl"] = int(shelved_cl)
-
-                # For shelved changelists, check that the revision of each file that the
-                # shelve was based on matches the revision that we are using for the
-                # starting point for git-fast-import (self.initialParent). Otherwise
-                # the resulting diff will contain deltas from multiple commits.
-
-                if file["action"] != "add" and \
-                    not self.cmp_shelved(path, file["rev"], origin_revision):
-                    sys.exit("change {0} not based on {1} for {2}, cannot unshelve".format(
-                        commit["change"], self.initialParent, path))
-
             files.append(file)
             fnum = fnum + 1
         return files
@@ -3032,7 +3010,7 @@ def hasBranchPrefix(self, path):
             print('Ignoring file outside of prefix: {0}'.format(path))
         return hasPrefix
 
-    def commit(self, details, files, branch, parent = ""):
+    def commit(self, details, files, branch, parent = "", allow_empty=False):
         epoch = details["time"]
         author = details["user"]
         jobs = self.extractJobsFromCommit(details)
@@ -3046,7 +3024,10 @@ def commit(self, details, files, branch, parent = ""):
         files = [f for f in files
             if self.inClientSpec(f['path']) and self.hasBranchPrefix(f['path'])]
 
-        if not files and not gitConfigBool('git-p4.keepEmptyCommits'):
+        if gitConfigBool('git-p4.keepEmptyCommits'):
+            allow_empty = True
+
+        if not files and not allow_empty:
             print('Ignoring revision {0} as it would produce an empty commit.'
                 .format(details['change']))
             return
@@ -3387,10 +3368,10 @@ def searchParent(self, parent, branch, target):
         else:
             return None
 
-    def importChanges(self, changes, shelved=False, origin_revision=0):
+    def importChanges(self, changes, origin_revision=0):
         cnt = 1
         for change in changes:
-            description = p4_describe(change, shelved)
+            description = p4_describe(change)
             self.updateOptionDict(description)
 
             if not self.silent:
@@ -3460,7 +3441,7 @@ def importChanges(self, changes, shelved=False, origin_revision=0):
                                 print("Parent of %s not found. Committing into head of %s" % (branch, parent))
                             self.commit(description, filesForCommit, branch, parent)
                 else:
-                    files = self.extractFilesFromCommit(description, shelved, change, origin_revision)
+                    files = self.extractFilesFromCommit(description)
                     self.commit(description, files, self.branch,
                                 self.initialParent)
                     # only needed once, to connect to the previous commit
@@ -3957,7 +3938,6 @@ def __init__(self):
         self.verbose = False
         self.noCommit = False
         self.destbranch = "refs/remotes/p4-unshelved"
-        self.origin = "p4/master"
 
     def renameBranch(self, branch_name):
         """ Rename the existing branch to branch_name.N
@@ -3989,6 +3969,32 @@ def findLastP4Revision(self, starting_point):
 
         sys.exit("could not find git-p4 commits in {0}".format(self.origin))
 
+    def createShelveParent(self, change, branch_name, sync, origin):
+        """ Create a commit matching the parent of the shelved changelist 'change'
+        """
+        parent_description = p4_describe(change, shelved=True)
+        parent_description['desc'] = 'parent for shelved changelist {}\n'.format(change)
+        files = sync.extractFilesFromCommit(parent_description, shelved=False, shelved_cl=change)
+
+        parent_files = []
+        for f in files:
+            # if it was added in the shelved changelist, it won't exist in the parent
+            if f['action'] in self.add_actions:
+                continue
+
+            # if it was deleted in the shelved changelist it must not be deleted
+            # in the parent - we might even need to create it if the origin branch
+            # does not have it
+            if f['action'] in self.delete_actions:
+                f['action'] = 'add'
+
+            parent_files.append(f)
+
+        sync.commit(parent_description, parent_files, branch_name,
+                parent=origin, allow_empty=True)
+        print("created parent commit for {0} based on {1} in {2}".format(
+            change, self.origin, branch_name))
+
     def run(self, args):
         if len(args) != 1:
             return False
@@ -3998,9 +4004,8 @@ def run(self, args):
 
         sync = P4Sync()
         changes = args
-        sync.initialParent = self.origin
 
-        # use the first change in the list to construct the branch to unshelve into
+        # only one change at a time
         change = changes[0]
 
         # if the target branch already exists, rename it
@@ -4013,14 +4018,21 @@ def run(self, args):
         sync.suppress_meta_comment = True
 
         settings = self.findLastP4Revision(self.origin)
-        origin_revision = settings['change']
         sync.depotPaths = settings['depot-paths']
         sync.branchPrefixes = sync.depotPaths
 
         sync.openStreams()
         sync.loadUserMapFromCache()
         sync.silent = True
-        sync.importChanges(changes, shelved=True, origin_revision=origin_revision)
+
+        # create a commit for the parent of the shelved changelist
+        self.createShelveParent(change, branch_name, sync, self.origin)
+
+        # create the commit for the shelved changelist itself
+        description = p4_describe(change, True)
+        files = sync.extractFilesFromCommit(description, True, change)
+
+        sync.commit(description, files, branch_name, "")
         sync.closeStreams()
 
         print("unshelved changelist {0} into {1}".format(change, branch_name))
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index c3d15ceea8..98beadffe4 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -19,8 +19,10 @@ test_expect_success 'init depot' '
 		p4 add file1 &&
 		p4 submit -d "change 1" &&
 		: >file_to_delete &&
+		: >file_to_move &&
 		p4 add file_to_delete &&
-		p4 submit -d "file to delete"
+		p4 add file_to_move &&
+		p4 submit -d "add files to delete"
 	)
 '
 
@@ -36,6 +38,8 @@ test_expect_success 'create shelved changelist' '
 		echo "new file" >file2 &&
 		p4 add file2 &&
 		p4 delete file_to_delete &&
+		p4 edit file_to_move &&
+		p4 move file_to_move moved_file &&
 		p4 opened &&
 		p4 shelve -i <<EOF
 Change: new
@@ -47,6 +51,8 @@ Files:
 	//depot/file1
 	//depot/file2
 	//depot/file_to_delete
+	//depot/file_to_move
+	//depot/moved_file
 EOF
 
 	) &&
@@ -59,7 +65,9 @@ EOF
 		test_path_is_file file2 &&
 		test_cmp file1 "$cli"/file1 &&
 		test_cmp file2 "$cli"/file2 &&
-		test_path_is_missing file_to_delete
+		test_path_is_missing file_to_delete &&
+		test_path_is_missing file_to_move &&
+		test_path_is_file moved_file
 	)
 '
 
@@ -92,6 +100,18 @@ EOF
 	)
 '
 
+shelve_one_file () {
+	description="Change to be unshelved" &&
+	file="$1" &&
+	p4 shelve -i <<EOF
+Change: new
+Description:
+	$description
+Files:
+	$file
+EOF
+}
+
 # This is the tricky case where the shelved changelist base revision doesn't
 # match git-p4's idea of the base revision
 #
@@ -108,29 +128,52 @@ test_expect_success 'create shelved changelist based on p4 change ahead of p4/ma
 		p4 submit -d "change:foo" &&
 		p4 edit file1 &&
 		echo "bar" >>file1 &&
-		p4 shelve -i <<EOF &&
-Change: new
-Description:
-	Change to be unshelved
-Files:
-	//depot/file1
-EOF
+		shelve_one_file //depot/file1 &&
 		change=$(last_shelved_change) &&
-		p4 describe -S $change | grep -q "Change to be unshelved"
+		p4 describe -S $change >out.txt &&
+		grep -q "Change to be unshelved" out.txt
 	)
 '
 
-# Now try to unshelve it. git-p4 should refuse to do so.
+# Now try to unshelve it.
 test_expect_success 'try to unshelve the change' '
 	test_when_finished cleanup_git &&
 	(
 		change=$(last_shelved_change) &&
 		cd "$git" &&
-		test_must_fail git p4 unshelve $change 2>out.txt &&
-		grep -q "cannot unshelve" out.txt
+		git p4 unshelve $change >out.txt &&
+		grep -q "unshelved changelist $change" out.txt
 	)
 '
 
+# Specify the origin. Create 2 unrelated files, and check that
+# we only get the one in HEAD~, not the one in HEAD.
+
+test_expect_success 'unshelve specifying the origin' '
+	(
+		cd "$cli" &&
+		: >unrelated_file0 &&
+		p4 add unrelated_file0 &&
+		p4 submit -d "unrelated" &&
+		: >unrelated_file1 &&
+		p4 add unrelated_file1 &&
+		p4 submit -d "unrelated" &&
+		: >file_to_shelve &&
+		p4 add file_to_shelve &&
+		shelve_one_file //depot/file_to_shelve
+	) &&
+	git p4 clone --dest="$git" //depot/@all &&
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		change=$(last_shelved_change) &&
+		git p4 unshelve --origin HEAD~ $change &&
+		git checkout refs/remotes/p4-unshelved/$change &&
+		test_path_is_file unrelated_file0 &&
+		test_path_is_missing unrelated_file1 &&
+		test_path_is_file file_to_shelve
+	)
+'
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
2.19.1.272.gf84b9b09d4

Re: [PATCHv1 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> The branch detection code looks for branches under refs/remotes/p4/...
> and can end up getting confused if there are unshelved changes in
> there as well. This happens in the function p4BranchesInGit().
>
> Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.

I am not a p4 user (and not a git-p4 user), so it is a bit hard for
me to assess if this is a backward incompatibile change and if so
how serious potential breakage to existing users would be.

>  
> -If the target branch in refs/remotes/p4/unshelved already exists, the old one will
> +If the target branch in refs/remotes/p4-unshelved already exists, the old one will
>  be renamed.
>  
>  ----
>  $ git p4 sync
>  $ git p4 unshelve 12345
> -$ git show refs/remotes/p4/unshelved/12345
> +$ git show p4/unshelved/12345

Isn't this "p4-unshelved/12345" now?

Re: [PATCHv1 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Luke Diamand]

On Fri, 12 Oct 2018 at 14:45, Junio C Hamano <gitster@pobox.com> wrote:
>
> Luke Diamand <luke@diamand.org> writes:
>
> > The branch detection code looks for branches under refs/remotes/p4/...
> > and can end up getting confused if there are unshelved changes in
> > there as well. This happens in the function p4BranchesInGit().
> >
> > Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.
>
> I am not a p4 user (and not a git-p4 user), so it is a bit hard for
> me to assess if this is a backward incompatibile change and if so
> how serious potential breakage to existing users would be.

I don't think it's a particularly serious breakage - it reports the
branch it unshelves to, so it should be fairly obvious.

However, maybe it would make sense to pull this into a separate commit
to make it more obvious? I should have thought of that before
submitting.

>
> >
> > -If the target branch in refs/remotes/p4/unshelved already exists, the old one will
> > +If the target branch in refs/remotes/p4-unshelved already exists, the old one will
> >  be renamed.
> >
> >  ----
> >  $ git p4 sync
> >  $ git p4 unshelve 12345
> > -$ git show refs/remotes/p4/unshelved/12345
> > +$ git show p4/unshelved/12345
>
> Isn't this "p4-unshelved/12345" now?

Yes, I think another reason to pull into a separate commit.

Luke

>

Re: [PATCHv1 2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved

[Luke Diamand]

On Fri, 12 Oct 2018 at 19:19, Luke Diamand <luke@diamand.org> wrote:
>
> On Fri, 12 Oct 2018 at 14:45, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Luke Diamand <luke@diamand.org> writes:
> >
> > > The branch detection code looks for branches under refs/remotes/p4/...
> > > and can end up getting confused if there are unshelved changes in
> > > there as well. This happens in the function p4BranchesInGit().
> > >
> > > Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.
> >
> > I am not a p4 user (and not a git-p4 user), so it is a bit hard for
> > me to assess if this is a backward incompatibile change and if so
> > how serious potential breakage to existing users would be.
>
> I don't think it's a particularly serious breakage - it reports the
> branch it unshelves to, so it should be fairly obvious.
>
> However, maybe it would make sense to pull this into a separate commit
> to make it more obvious? I should have thought of that before
> submitting.
>
> >
> > >
> > > -If the target branch in refs/remotes/p4/unshelved already exists, the old one will
> > > +If the target branch in refs/remotes/p4-unshelved already exists, the old one will
> > >  be renamed.
> > >
> > >  ----
> > >  $ git p4 sync
> > >  $ git p4 unshelve 12345
> > > -$ git show refs/remotes/p4/unshelved/12345
> > > +$ git show p4/unshelved/12345
> >
> > Isn't this "p4-unshelved/12345" now?
>
> Yes, I think another reason to pull into a separate commit.

D'oh. It's already in a separate commit.
I'll re-roll fixing that documentation.

I think it will be fine to change the branch that the unshelving
happens into - I think it's very unlikely anyone is basing some
automated scripts on this, because of the way that unshelving is used
anyway.

Luke