On 12/01/2023 12:25, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f9675bd24e6..511ace43db0 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue
rebasing.
To interrupt the rebase (just like an "edit" command would do, but without
-cherry-picking any commit first), use the "break" command.
+cherry-picking any commit first), use the "break" command. A "break"
+command may be followed by a comment beginning with `#` followed by a
+space.
You're missing a corresponding edit here to the help string in
append_todo_help(), as you note you're making "break" support what
"merge" does, and that help string documents that "merge" accepts a
comment, after this we don't do that for "break", but should one way or
the other (see below).
Thanks, Andrei has already mentioned that, I'll update the todo help
when I re-roll
I like this direction, but I don't see why we need to continue this
special-snowflakeness of only allowing specific commands to accept these
#-comments.
Why not just have them all support it? It started with "merge", which as
4c68e7ddb59 (sequencer: introduce the `merge` command, 2018-04-25) note
can be used for:
merge -C baaabaaa abc # Merge the branch 'abc' into master
As Olliver points out we should probably support "#" without the
following " ", which seems to be an accident of history &
over-strictness.
It's not an accident, labels and commit names can begin with '#' so we
need to support
merge #parent1 #parent2
For "break" we could just not require '#' at all as we do for "reset
<label>" where anything following the label is ignored. That would mean
we couldn't add an argument to break in the future though (I'm not sure
that is really a problem in practice). If we're going to require '#'
then we may as well following the existing rules.
But in this commit you extend it to "break", but we're going out of or
way to e.g. extend this to "noop".
I'm struggling to see why "noop" would need a comment - it only exists
to avoid an empty todo list and is not meant for general use (it's not
in the help added by append_todo_help() for this reason)
For "pick", "edit", "reword", "fixup" & "squash" we don't need a comment
mechanism as we ignore everything after the commit name. For "reset" we
ignore everything after the label. For "label" we could add support for
comments but I'm not sure it is that useful and we'd need to be careful
not to interpret a bad label name as a label + comment.
So I'd expect that just like the first for-loop in "parse_insn_line()"
we'd check if strchr(bol, '#') returns non-NULL, and if so set eol to
that result.
That would break labels and commit names that contain a '#'
If we think we're never going to want "break" to take an argument then
maybe we should just make it ignore the rest of the line like "reset
<label>".
Best Wishes
Phillip
The "just like" being that we may want to explicitly forbid this or
handle it specially for some, e.g. I didn't check but do the "label" and
"reset" perhaps support arbitrary non-'\n' (probably by accident, and we
could support commments there too).
For "pick" we probably need to explicitly exclude it, although I can't
remember if we do anything useful with the part of the line after the
OID (maybe not...).
