Thread (2 messages) 2 messages, 2 authors, 2023-10-30

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

From: Elijah Newren <hidden>
Date: 2023-10-30 17:26:34

On Sun, Oct 29, 2023 at 9:01 PM Junio C Hamano [off-list ref] wrote:
quoted hunk ↗ jump to hunk
"Tony Tung via GitGitGadget" [off-list ref] writes:
quoted
From: Tony Tung <redacted>

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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help