On Thu, Jan 12, 2023 at 9:20 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > On Thu, Jan 12 2023, Phillip Wood wrote: > > > On 12/01/2023 12:25, Ævar Arnfjörð Bjarmason wrote: > >> On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote: > >> [...] > >> 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) > > I'm struggling to see why "break" needs a comment, why not just add it > to the preceding line or something? But it seems some users like it :) > > So at that point, it seems easier to both explain & implement something > that just consistently supports comment syntax, rather than overly > special-casing it. Personally, I think we should allow all lines in the todo script to have trailing comments. > > 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. > > I think there's been a couple of request to have changing the "argument" > actually reword the $subject (I'm pretty such for "reword" that got as > far as a patch, but I may be misrecalling). Yes, there have been, but it's a bad idea (and it's not just me saying that; Junio has also declared it as such). I think it's mostly based on confusion from us having ignored non-commented stuff on a line, and we could avoid that confusion by just commenting out the commit subject to make it clear we will ignore it and any changes to it. > >> 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>". > > It's unfortunate that we do that, I think it's almost always better to > just error out rather that silently ignore data, except for some > explicit exceptions (such as comment syntax). I agree.
