On Tue, 12 May 2020 at 21:05, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Ben Keene via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > > > 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
