Re: [PATCH v3] sequencer: use configured comment character
Hi Daniel, On Mon, 16 Jul 2018, Daniel Harding wrote: > On Mon, 16 Jul 2018 at 18:59:03 +0300, Johannes Schindelin wrote: > > > > On Mon, 16 Jul 2018, Aaron Schrab wrote: > > > > > > Looking into that a bit further, it does seem like my explanation above > > > was incorrect. Here's another attempt to explain why setting > > > core.commentChar=auto isn't a problem for this change. > > > > > > 8< - > > > > > > 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. > > > > > > Setting core.commentChar to "auto" will not be honored here, and the > > > previously configured or default value will be used instead. But, since > > > the todo list will consist of only generated content, there should not > > > be any non-comment lines beginning with that character. > > > > How about this instead? > > > > If core.commentChar is set to "auto", the intention is to > > determine the comment line character from whatever content is there > > already. > > > > As the code path in question is the one *generating* the todo list > > from scratch, it will automatically use whatever core.commentChar > > has been configured before the "auto" (and fall back to "#" if none > > has been configured explicitly), which is consistent with users' > > expectations. > > Honestly, the above still doesn't read clearly to me. I've take a stab at it > myself - let me know what you think: > > If core.commentChar is set to "auto", the comment_line_char global > variable will be initialized to '#'. The only time > comment_line_char gets changed to an automatic value is when the > prepare_to_commit() function (in commit.c) calls > adjust_comment_line_char(). This does not happen when generating > the todo list, so '#' will be used as the comment character in the > todo list if core.commentChar is set to "auto". There is a concocted way to have core.commentChar = auto *and* to override the comment char: if you use `git config --add core.commentChar auto`, or if you have it set in $HOME/.gitconfig and in .git/config. I tried to cover that in my suggestion, but that was probably trying to be too precise, rather than being useful... Ciao, Johannes
Re: [PATCH v3] sequencer: use configured comment character
Hi Johannes, On Mon, 16 Jul 2018 at 18:59:03 +0300, Johannes Schindelin wrote: Hi Aaron, On Mon, 16 Jul 2018, Aaron Schrab wrote: Looking into that a bit further, it does seem like my explanation above was incorrect. Here's another attempt to explain why setting core.commentChar=auto isn't a problem for this change. 8< - 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. Setting core.commentChar to "auto" will not be honored here, and the previously configured or default value will be used instead. But, since the todo list will consist of only generated content, there should not be any non-comment lines beginning with that character. How about this instead? If core.commentChar is set to "auto", the intention is to determine the comment line character from whatever content is there already. As the code path in question is the one *generating* the todo list from scratch, it will automatically use whatever core.commentChar has been configured before the "auto" (and fall back to "#" if none has been configured explicitly), which is consistent with users' expectations. Honestly, the above still doesn't read clearly to me. I've take a stab at it myself - let me know what you think: If core.commentChar is set to "auto", the comment_line_char global variable will be initialized to '#'. The only time comment_line_char gets changed to an automatic value is when the prepare_to_commit() function (in commit.c) calls adjust_comment_line_char(). This does not happen when generating the todo list, so '#' will be used as the comment character in the todo list if core.commentChar is set to "auto". Cheers, Daniel Harding
[PATCH v3] sequencer: use configured comment character
At 10:15 -0700 12 Jul 2018, Junio C Hamano wrote: >Aaron Schrab writes: >> 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? Looking into that a bit further, it does seem like my explanation above was incorrect. Here's another attempt to explain why setting core.commentChar=auto isn't a problem for this change. 8< - 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. Setting core.commentChar to "auto" will not be honored here, and the previously configured or default value will be used instead. But, since the todo list will consist of only generated content, there should not be any non-comment lines beginning with that character. Signed-off-by: Aaron Schrab --- 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(, >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"); -- 2.18.0.419.gfe4b301394
