"Alisha Kim via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Alisha Kim <pril@pril.cc>
>
> 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.

> @@ -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.

>              except IOError:
>                  print(self.gitError.read())
>                  sys.exit(1)
>
> base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169

On Thu, Nov 23, 2023 at 10:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Alisha Kim via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Alisha Kim <pril@pril.cc>
> >
> > 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