From: Junio C Hamano <gitster@pobox.com>
To: Luke Diamand <luke@diamand.org>, Ben Keene <seraphire@gmail.com>,
Andrew Oakley <andrew@adoakley.name>
Cc: git@vger.kernel.org, Evan McLain <git.commit@none-of-yer.biz>,
"Evan McLain via GitGitGadget" <gitgitgadget@gmail.com>
Subject: Re: [PATCH] git-p4: fix "git p4 sync" after ignored changelist
Date: Mon, 10 May 2021 13:10:27 +0900 [thread overview]
Message-ID: <xmqqsg2vrzyk.fsf@gitster.g> (raw)
In-Reply-To: <pull.941.git.1620490353758.gitgitgadget@gmail.com> (Evan McLain via GitGitGadget's message of "Sat, 08 May 2021 16:12:33 +0000")
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
next prev parent reply other threads:[~2021-05-10 4:10 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-08 16:12 [PATCH] git-p4: fix "git p4 sync" after ignored changelist Evan McLain via GitGitGadget
2021-05-10 4:10 ` Junio C Hamano [this message]
2021-05-10 17:36 ` Andrew Oakley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqsg2vrzyk.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=andrew@adoakley.name \
--cc=git.commit@none-of-yer.biz \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=luke@diamand.org \
--cc=seraphire@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).