Re: [GSOC PATCH v2] commit: avoid scanning trailing comments when 'core.commentChar' is "auto"

[Ayush Chandekar <ayu.chandekar@gmail.com>]

On Fri, Jun 27, 2025 at 2:04 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> >       if (!memchr(sb->buf, candidates[0], sb->len)) {
> >               free(comment_line_str_to_free);
> > @@ -700,7 +704,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
> >       candidate = strchr(candidates, *p);
> >       if (candidate)
> >               *candidate = ' ';
> > -     for (p = sb->buf; *p; p++) {
> > +     for (p = sb->buf; p + 1 < sb->buf + cutoff; p++) {
> >               if ((p[0] == '\n' || p[0] == '\r') && p[1]) {
> >                       candidate = strchr(candidates, p[1]);
> >                       if (candidate)
> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > index 127216f722..ccfe77af6c 100755
> > --- a/t/t3418-rebase-continue.sh
> > +++ b/t/t3418-rebase-continue.sh
> > @@ -328,6 +328,20 @@ test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
> >       test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
> >   '
> >
> > +test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' '
> > +     test_commit base file &&
>
> If you used an existing file (F1 or F2) like most of the rest of the
> tests in this file we could avoid creating this commit and save
> ourselves a couple of processes.
>

Yeah, right, I'll update it.

> > +     git checkout -b branch-a &&
> > +     test_commit A file &&
> > +     git checkout -b branch-b base &&
> > +     test_commit B file &&
> > +     test_must_fail git rebase branch-a &&
> > +     printf "B\nA\n" >file &&
> > +     git add file &&
> > +     GIT_EDITOR="cat >actual" git -c core.commentChar=auto rebase --continue &&
> > +     # Check that "#" is still the comment character.
> > +     test_grep "^# Changes to be committed:$" actual
>
> I agree that it is a good idea to anchor the start of the message, but
> I'm not sure it is helpful to anchor the end of the message as we don't
> want the test to fail just because an unrelated change adds some
> whitespace to the end of this line. I'd be tempted to drop the ':' for
> the same reason.
>

Makes sense, I'll fix it.
Thanks a lot for reviewing!