Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2018, 17:15 UTC
Message-ID: <xmqq4lh4z870.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180712030249.22071-1-aaron@schrab.com>

Aaron Schrab <aaron@schrab.com> writes:

Show 5 quoted lines

> Subject: [PATCH v2] sequencer: use configured comment character
>
> Use the configured comment character when generating comments about
> branches in a todo list.  Failure to honor this configuration causes a
> failure to parse the resulting todo list.

OK.

>
> Note that the comment_line_char has already been resolved by this point,
> even if the user has configured the comment character to be selected
> automatically.

Isn't this a slight lie?

The core.commentchar=auto setting is noticed by everybody (including the users of the sequencer machinery), but it is honored only by builtin/commit.c::prepare_to_commit() that is called by builtin/commit.c::cmd_commit(), i.e. the implementation of "git commit" that should not be used as a subroutine by other commands, and by nothing else. If the user has core.commentchar=auto, the comment_line_char is left to the default '#' in the sequencer codepath.

I think the patch is still correct and safe, but the reason why it is so is not because we chose a suitable character (that is how I read what "has already been resolved by this point" means) by calling builtin/commit.c::adjust_comment_line_char(). Isn't it because the "script" the function is working on does not have a line that came from arbitrary end-user input that may happen to begin with '#', hence the default '#' is safe to use?

Show 17 quoted lines

> Signed-off-by: Aaron Schrab <aaron@schrab.com>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 4034c0461b..caf91af29d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  		entry = oidmap_get(&state.commit2label, &commit->object.oid);
>  
>  		if (entry)
> -			fprintf(out, "\n# Branch %s\n", entry->string);
> +			fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
>  		else
>  			fprintf(out, "\n");