Re: [PATCH] git-p4: fix "git p4 sync" after ignored changelist

[Junio C Hamano <gitster@pobox.com>]

Even to somebody like me who does not know P4 all that much, the log
message does indicate that it is written by somebody who knows what
s/he is talking about, but asking help evaluating, or better yet,
giving Acks, from those who had contributions in git-p4 in the past
12 months.

Thanks.

"Evan McLain via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Evan McLain <git.commit@none-of-yer.biz>
>
> After a sync/clone, if the very next p4 change is outside the client
> view (and therefore "empty" as far as git-p4 is concerned), a
> subsequent sync will fail with a message like:
>
>      "fast-import failed: warning: Not updating refs/remotes/p4/master
>       (new tip xxxxxx does not contain yyyyyy)"
>
> The bug is caused by discarding the parent commit information
> unconditionally after processing a p4 change, whether the change was
> committed or not.  This leaves the next non-empty commit disconnected
> from its parent, causing fast-import to fail.  This only occurs when
> git-p4.keepEmptyCommits is false, but that is the default.
>
> Rename P4Sync.commit() to maybeCommit() and return True if the change
> is committed, or False if ignored. Clear P4Sync.initialParent only if
> maybeCommit() returns True.
>
> Diagnosed by IanH at https://stackoverflow.com/a/39876288/2033415
>
> Signed-off-by: Evan McLain <git.commit@none-of-yer.biz>
> ---
>     git-p4: fix "git p4 sync" after ignored changelist
>     
>     After a sync/clone, if the very next p4 change is outside the client
>     view (and therefore "empty" as far as git-p4 is concerned), a subsequent
>     sync will fail with a message like:
>     
>      "fast-import failed: warning: Not updating refs/remotes/p4/master
>       (new tip xxxxxx does not contain yyyyyy)"
>     
>     
>     The bug is caused by discarding the parent commit information
>     unconditionally after processing a p4 change, whether the change was
>     committed or not. This leaves the next non-empty commit disconnected
>     from its parent, causing fast-import to fail. This only occurs when
>     git-p4.keepEmptyCommits is false, but that is the default.
>     
>     Rename P4Sync.commit() to maybeCommit() and return True if the change is
>     committed, or False if ignored. Clear P4Sync.initialParent only if
>     maybeCommit() returns True.
>     
>     Diagnosed by IanH at https://stackoverflow.com/a/39876288/2033415
>     
>     Signed-off-by: Evan McLain git.commit@none-of-yer.biz
>     
>     ===
>     
>     There may be some other latent bugs here that I haven't fixed. In
>     particular, there seems to be a similar flow when detecting branches
>     with del self.initialParents[branch]. I wasn't sure how to set up a
>     repro case to expose that error, so I just fixed the bug I understood.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-941%2Femclain%2Fem%2Ffix-p4-sync-after-ignored-change-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-941/emclain/em/fix-p4-sync-after-ignored-change-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/941
>
>  git-p4.py                     | 29 +++++++++++++++------------
>  t/t9809-git-p4-client-view.sh | 37 +++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 13 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 09c9e93ac401..f15818e1a842 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -3251,7 +3251,9 @@ def findShadowedFiles(self, files, change):
>                      'rev': record['headRev'],
>                      'type': record['headType']})
>  
> -    def commit(self, details, files, branch, parent = "", allow_empty=False):
> +    # Commit a p4 change to git, unless it should be ignored.
> +    # Returns True if the change was committed to git, or False if it was ignored.
> +    def maybeCommit(self, details, files, branch, parent = "", allow_empty=False):
>          epoch = details["time"]
>          author = details["user"]
>          jobs = self.extractJobsFromCommit(details)
> @@ -3274,7 +3276,7 @@ def commit(self, details, files, branch, parent = "", allow_empty=False):
>          if not files and not allow_empty:
>              print('Ignoring revision {0} as it would produce an empty commit.'
>                  .format(details['change']))
> -            return
> +            return False
>  
>          self.gitStream.write("commit %s\n" % branch)
>          self.gitStream.write("mark :%s\n" % details["change"])
> @@ -3340,6 +3342,7 @@ def commit(self, details, files, branch, parent = "", allow_empty=False):
>                  if not self.silent:
>                      print("Tag %s does not match with change %s: file count is different."
>                             % (labelDetails["label"], change))
> +        return True
>  
>      # Build a dictionary of changelists and labels, for "detect-labels" option.
>      def getLabels(self):
> @@ -3676,22 +3679,22 @@ def importChanges(self, changes, origin_revision=0):
>                              tempBranch = "%s/%d" % (self.tempBranchLocation, change)
>                              if self.verbose:
>                                  print("Creating temporary branch: " + tempBranch)
> -                            self.commit(description, filesForCommit, tempBranch)
> +                            self.maybeCommit(description, filesForCommit, tempBranch)
>                              self.tempBranches.append(tempBranch)
>                              self.checkpoint()
>                              blob = self.searchParent(parent, branch, tempBranch)
>                          if blob:
> -                            self.commit(description, filesForCommit, branch, blob)
> +                            self.maybeCommit(description, filesForCommit, branch, blob)
>                          else:
>                              if self.verbose:
>                                  print("Parent of %s not found. Committing into head of %s" % (branch, parent))
> -                            self.commit(description, filesForCommit, branch, parent)
> +                            self.maybeCommit(description, filesForCommit, branch, parent)
>                  else:
>                      files = self.extractFilesFromCommit(description)
> -                    self.commit(description, files, self.branch,
> -                                self.initialParent)
> -                    # only needed once, to connect to the previous commit
> -                    self.initialParent = ""
> +                    if self.maybeCommit(description, files, self.branch,
> +                                        self.initialParent):
> +                        # only needed once, to connect to the previous commit
> +                        self.initialParent = ""
>              except IOError:
>                  print(self.gitError.read())
>                  sys.exit(1)
> @@ -3755,7 +3758,7 @@ def importHeadRevision(self, revision):
>  
>          self.updateOptionDict(details)
>          try:
> -            self.commit(details, self.extractFilesFromCommit(details), self.branch)
> +            self.maybeCommit(details, self.extractFilesFromCommit(details), self.branch)
>          except IOError as err:
>              print("IO error with git fast-import. Is your git version recent enough?")
>              print("IO error details: {}".format(err))
> @@ -4265,8 +4268,8 @@ def createShelveParent(self, change, branch_name, sync, origin):
>  
>              parent_files.append(f)
>  
> -        sync.commit(parent_description, parent_files, branch_name,
> -                parent=origin, allow_empty=True)
> +        sync.maybeCommit(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))
>  
> @@ -4307,7 +4310,7 @@ def run(self, args):
>          description = p4_describe(change, True)
>          files = sync.extractFilesFromCommit(description, True, change)
>  
> -        sync.commit(description, files, branch_name, "")
> +        sync.maybeCommit(description, files, branch_name, "")
>          sync.closeStreams()
>  
>          print("unshelved changelist {0} into {1}".format(change, branch_name))
> diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
> index 9c9710d8c7b8..4daf0305474c 100755
> --- a/t/t9809-git-p4-client-view.sh
> +++ b/t/t9809-git-p4-client-view.sh
> @@ -147,6 +147,43 @@ test_expect_success 'later mapping takes precedence (partial repo)' '
>  	git_verify $files
>  '
>  
> +# after a sync/clone of a partial client view, if the very next p4 change is outside
> +# the view and thus ignored, the a subsequent sync will fail to connect to its parent:
> +#     "fast-import failed: warning: Not updating refs/remotes/p4/master
> +#      (new tip x does not contain y)"
> +test_expect_success 'partial sync bug with ignored change' '
> +	client_view "//depot/dir1/... //client/..." &&
> +	files="file11 file12" &&
> +	client_verify $files &&
> +	test_when_finished cleanup_git &&
> +	git p4 clone --use-client-spec --dest="$git" //depot &&
> +	cli2="$TRASH_DIRECTORY/cli2" &&
> +	mkdir -p "$cli2" &&
> +	test_when_finished "p4 client -f -d client2 && rm -rf \"$cli2\"" &&
> +	(
> +		cd "$cli2" &&
> +		P4CLIENT=client2 &&
> +		cli="$cli2" &&
> +		client_view "//depot/... //client2/..." &&
> +		p4 sync &&
> +		p4 open dir2/file21 &&
> +		echo dir2/file21 update >dir2/file21 &&
> +		p4 submit -d "update dir2/file21"
> +	) &&
> +	(
> +		cd "$cli" &&
> +		p4 sync &&
> +		p4 open file11 &&
> +		echo file11 update >file11 &&
> +		p4 submit -d "update file11"
> +	) &&
> +	(
> +		cd "$git" &&
> +		git p4 sync --use-client-spec
> +	) &&
> +	git_verify $files
> +'
> +
>  # Reading the view backwards,
>  #   dir2 goes to cli12
>  #   dir1 cannot go to cli12 since it was filled by dir2
>
> base-commit: 48bf2fa8bad054d66bd79c6ba903c89c704201f7