Re: [PATCH] sequencer: remove use of comment character

From: Elijah Newren <newren@gmail.com>
Date: Oct 30, 2023, 17:26 UTC
Message-ID: <CABPp-BE6_nuMeiqOAMGwP8SH=d1+i57-STgTNKU8-Gnkv2jW=Q@mail.gmail.com>
In-Reply-To: <xmqq7cn4g3nx.fsf@gitster.g>

On Sun, Oct 29, 2023 at 9:01 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 62 quoted lines

>
> "Tony Tung via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Tony Tung <tonytung@merly.org>
> >
> > Instead of using the hardcoded `# `, use the
> > user-defined comment_line_char.  Adds a test
> > to prevent regressions.
>
> Good spotting.
>
> Two observations.
>
>  (1) There are a few more places that need similar treatment in the
>      same file; you may want to fix them all while at it.
>
>  (2) The second argument to strbuf_commented_addf() is always the
>      comment_line_char global variable, not just inside this file
>      but all callers across the codebase.  We probably should drop
>      it and have the strbuf_commented_addf() helper itself refer to
>      the global.  That way, if we ever want to change the global
>      variable reference to something else (e.g. function call), we
>      only have to touch a single place.
>
> The latter is meant as #leftoverbits and will be a lot wider
> clean-up that we may want to do long after this patch hits out
> codebase.  The "other places" I spotted for the former are the
> following, but needs to be taken with a huge grain of salt, as it
> has not even been compile tested.
>
> Thanks.
>
>  sequencer.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git c/sequencer.c w/sequencer.c
> index d584cac8ed..33208b1660 100644
> --- c/sequencer.c
> +++ w/sequencer.c
> @@ -1893,8 +1893,8 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
>         size_t orig_msg_len;
>         int i = 1;
>
> -       strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
> -       strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
> +       strbuf_addf(&buf1, comment_line_char, "%s\n", _(first_commit_msg_str));
> +       strbuf_addf(&buf2, comment_line_char, "%s\n", _(skip_first_commit_msg_str));
>         s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
>         while (s) {
>                 const char *next;
> @@ -2269,8 +2269,8 @@ static int do_pick_commit(struct repository *r,
>                 next = parent;
>                 next_label = msg.parent_label;
>                 if (opts->commit_use_reference) {
> -                       strbuf_addstr(&msgbuf,
> -                               "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> +                       strbuf_commented_addf(&msgbuf, comment_line_char, "%s",
> +                               "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
>                 } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
>                            /*
>                             * We don't touch pre-existing repeated reverts, because
>

I thought the point of the comment_line_char was so that commit messages could have lines starting with '#'. That rationale doesn't apply to the TODO list generation or parsing, and I'm not sure if we want to add the same complexity there. If we do want to add the same complexity there, I'm worried that making these changes are insufficent; there are some other hardcoded '#' references in the code (as a quick greps for '".*#' and "'#'" will turn up). Since those other references include parsing as well as generation, I think we might actually be introducing bugs in the TODO list handling if we only partially convert it, but someone would need to double check.