Re: [PATCH] rebase -i: allow a comment after a "break" command

From: Jeff King <peff@peff.net>
Date: Jan 12, 2023, 16:46 UTC
Message-ID: <Y8A5X0kHE31kSH3z@coredump.intra.peff.net>
In-Reply-To: <xmqq358fu4vr.fsf@gitster.g>

On Fri, Jan 13, 2023 at 12:52:40AM +0900, Junio C Hamano wrote:

Show 16 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > When adding a "break" command to a rebase todo list it can be helpful to
> > add a comment as a reminder as to what the user was planning to do when
> > the rebase stopped. Anything following the command is interpreted as an
> > argument to the command and results in an error. Change this so that a
> > "break command may be followed by "# <comment>" in the same way as
> > a "merge" command. Requiring the comment to begin with "# " allows the
> > break command to start taking an argument in the future if that turns
> > out to be useful.
> 
> Why do we special case "break" and not give the same "comment is
> emitted when the control reaches the insn in the todo list" for
> others like "exec" or even "pick"?

I had somewhat the opposite thought. The "break" command is special in that it is not doing anything useful except returning control to the user. And hence producing a message is a useful add-on. So I expected the patch to just allow:

  break this is a message the user will see

without any "#" at all.

That does close the door for further arguments, but I have trouble imagining what they would be.

Show 8 quoted lines

> Another comment with devil's advocate hat on is if we are better off
> not adding "# this comment is emitted" at all, and instead do
> 
>     pick ...
>     pick ...
>     exec echo this comment is emitted
>     break
>     pick ...

Yeah, I have certainly done something like that before. I think it is just a matter of ergonomics. I'm pretty sure I have also forgotten to put in the "break" afterwards.

Actually, before we had "break" I think I've done:

  x false this message will be visible when this fails

which is even more horrible. ;)

-Peff