On Thu, Nov 23, 2023 at 10:57 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Alisha Kim via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: Alisha Kim <pril@xxxxxxx> > > > > When executing p4 sync by specifying an excluded path, an empty commit > > will be created if there is only a change in the excluded path in > > revision. > > If git-p4.keepEmptyCommits is turned off and an empty commit is the > > first, fast-import will fail. > > The above describe under what condition a failure gets triggered, > but there is no description on what approach the proposed solution > takes. You could teach "fast-import" to deal with an empty commit > properly, you could ignore empty commits and not produce input for > the fast-import command, you could probably turn these initial empty > commits into non-empty commits by adding dummy contents, etc. We > want to see in our proposed log messages what solution was taken and > how the solution was designed to satisfy what requirements. This is > to help future developers who will have to change the code that is > given by this patch, so that their updates can still adhere to what > ever design criteria you had in working on this change [*]. > > Side note: Your solution might be to ignore empty commits > despite keepEmptyCommits option is set (as I said, you did not > describe it at all in the above, so this is a hypothetical > example). If the reason behind choosing that design were "I > just do not want it to fail---I do not care if the resulting > history coming out of fast-import is crappy (I lose the p4 CL > descriptions for these commits, even though the user wants to > keep them)", then future developers can safely "fix" your fix > here by turning the initial empty commits into non-empty ones by > adding fake contents. I've added explanations for the changes I suggested. > > > > @@ -3876,10 +3878,12 @@ class P4Sync(Command, P4UserMap): > > self.commit(description, filesForCommit, branch, parent) > > else: > > files = self.extractFilesFromCommit(description) > > - self.commit(description, files, self.branch, > > + isCommitted = self.commit(description, files, self.branch, > > self.initialParent) > > # only needed once, to connect to the previous commit > > - self.initialParent = "" > > + if isCommitted: > > + self.initialParent = "" > > "is" does not sound grammatically correct. "didCommit" (i.e., "we > made a commit"), "haveCommitted" (i.e., "we have made a commit") > might be more understandable. The problem with correct grammar that you mentioned has also been changed. Thank you for your good comments. > > > > except IOError: > > print(self.gitError.read()) > > sys.exit(1) > > > > base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169
