From: Elijah Newren <newren@gmail.com>
Date: Mon, 29 Nov 2021 18:57:22 GMT
Subject: Re: [PATCH v11 2/2] am: support --empty=<option> to handle empty patches
Message-ID: <CABPp-BHJq6L_1dG_YELrgO9M0w6qS2Jg3gDtpVWonj9i9wUR4Q@mail.gmail.com>
In-Reply-To: <xmqqo8623jyw.fsf@gitster.g>

On Mon, Nov 29, 2021 at 10:17 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> +--empty=(die|drop|keep)::
> >> +       By default, or when the option is set to 'die', the command
> >> +       errors out on an input e-mail message that lacks a patch. When
> >> +       this option is set to 'drop', skip such an e-mail message instead.
> >> +       When this option is set to 'keep', create an empty commit,
> >> +       recording the contents of the e-mail message as its log.
> >
> > What does 'errors out' mean?  Is the am operation aborted, and the
> > user return to the pre-am state?  Or is the am operation interrupted,
> > with the user being asked to choose whether to keep or drop the patch?
>
> I think it is the same as how "git am" without this sees a piece of
> e-mail without any patch in it.  It exits with non-zero status, but
> keeps the contents of .git/rebase-apply directory intact so that the
> user can decide what the next action should be.  "the command stops
> with non-zero status and gives control back to the user" might be a
> better explanation.
>
> As to the name of the option's value, I think 'die' is a much better
> word than 'ask' for this behaviour.  It is not like we retain
> control, give "do you want to do X or Y?"  prompt and do either X or
> Y ourselves, which is what I expect out of 'ask'.  If we just exit
> with non-zero value and give the control back to the user, then we
> are not asking.

Well, I still think 'die' implies aborting the entire overall `am`
operation, and is thus a confusing term.  To me, 'stop' or 'interrupt'
would be better if you want a one-word term and don't like 'ask'.

If you don't like the term 'ask' here, perhaps we should also change
rebase?  rebase --empty has 'ask' as a choice for
stopping/interrupting the rebase operation and letting the user decide
how to handle it.  (There is a very subtly different context for
rebase's --empty=ask, namely in that it is only triggered for patches
that were not originally empty but become so do the the new base the
patches are being applied upon already having the changes from the
patch in question, but that doesn't seem like a big enough difference
to suggest it should have a different name.)


There's another bit to my query on the semantics, though.  The second
half of my critique was that the current form of the series does not
inform the user how to handle the situation when `git-am` stops.  It
only tells the user how to drop the patch.  (If there's only one
option, why doesn't git just take it automatically?)  I think it
should also tell the user how to keep the patch.  Now, if both options
are presented to the user when the operation stops, would that qualify
as the user having been 'asked' what to do?