Re: [PATCH v2] git-p4.py: fix --prepare-p4-only error with multiple commits

[Luke Diamand <luke@diamand.org>]

On Tue, 12 May 2020 at 21:05, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > ...
> > Be aware - this change will fix the existing test error in t9807.23
> > for --prepare-p4-only. However there is insufficent coverage for
> > this flag.  If more than 1 commit is pending submission to P4, the
> > method will properly prepare the P4 changelist, however it will
> > still exit the application with an exitcode of 1.
> >
> > The current documentation does not define what the exit code should be
> > in this condition.
> > (See: https://git-scm.com/docs/git-p4#Documentation/git-p4.txt---prepare-p4-only)
>
> Then some proposal to define what the behaviour should be is needed,
> the consensus implements and then documented.

It looks like it's always been slightly confusing (at least to me).

If prepare_p4_only is True, applyCommit() will only fail if the patch
could not be applied (which is what you would expect).

If that happens I would think it would make sense to bail out - I
would think most users will want to rebase and try again rather than
blithely carrying on.

The "i < last" logic in the original patch from 2012 seems to mean
that you only get the message "Processing only the first commit due to
option prepare-p4-only" if there is an error and you're not about to
drop out anyway.

I personally no longer use --prepare-p4-only, now that we have the
git-p4 shelve support. I would imagine this is true for a lot of
people.

The logic in Ben's change seems much more sensible.

Luke


>
> >
> > Signed-off-by: Ben Keene <seraphire@gmail.com>
> > ---
>
>
> >     git-p4.py: fix --prepare-p4-only error with multiple commits
> >
> >     When using git p4 submit with the --prepare-p4-only option, the program
> >     should prepare a single p4 changelist and notify the user that more
> >     commits are pending and then stop processing.
> >
> >     A bug has been introduced by the p4-changelist hook feature that causes
> >     the program to continue to try and process all pending changelists at
> >     the same time.
> >
> >     The function applyCommit should return True when applying the commit was
> >     successful and the program should continue. In the case of the
> >     --prepare-p4-only flag, the function should return False, alerting the
> >     caller that the program should not proceed with additional commits.
> >
> >     Change the return value from True to False in the applyCommit function
> >     when git-p4 is executed with --prepare-p4-only flag.
>
> This space below the three-dash-line is the best place to describe
> the difference between the previous version and this one.  It seems
> that the above text is not such a "here are what was bad/missing in
> v1 that got fixed/extended", or a copy of the log message (like many
> patches that come from GGG has).  I am a bit puzzled what it is, but
> for now let's pretend there wasn't any text below the three-dash-line
> and read on.
>
> >  git-p4.py | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/git-p4.py b/git-p4.py
> > index b8b2a1679e7..c4a4012bcc1 100755
> > --- a/git-p4.py
> > +++ b/git-p4.py
> > @@ -2537,11 +2537,12 @@ def run(self, args):
> >                  ok = self.applyCommit(commit)
> >              if ok:
> >                  applied.append(commit)
> > -            else:
> > -                if self.prepare_p4_only and i < last:
> > -                    print("Processing only the first commit due to option" \
> > -                          " --prepare-p4-only")
>
> So, it used to be that after failing to apply a commit, unless we
> are at the last step, we gave a message and left the loop under the
> prepare-p4-only mode.  We did not do anything special under the
> prepare-p4-only mode if applyCommit returned a success.
>
> > +                if self.prepare_p4_only:
> > +                    if i < last:
> > +                        print("Processing only the first commit due to option" \
> > +                                " --prepare-p4-only")
>
> Now, after successfully applying, we leave the loop under the
> prepare-p4-only mode.  We give the message only when we are not at
> the last step.
>
> >                      break
> > +            else:
>
> So..., what happens when the first step fails to apply and then the
> user tells us to skip the commit?  We'll go on to the next commit
> and then applyCommit() may say 'ok' this time around.  Does that
> count as "processed only the first commit and we are done"?
>
> >                  if i < last:
> >                      # prompt for what to do, or use the option/variable
> >                      if self.conflict_behavior == "ask":
> >
> > base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17