Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar
Aaron Schrab writes: > 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? > 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");
Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar
Sorry for taking so long to get back to this. Hopefully the following will be acceptable to everyone. 8< - 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. 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. 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
Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar
On Mon, 09 Jul 2018 at 10:53:14 +0300, Johannes Schindelin wrote> On Sun, 8 Jul 2018, Daniel Harding wrote: I have core.commentChar set in my .gitconfig, and when I tried to run git rebase -i -r, I received an error message like the following: error: invalid line 3: # Branch To fix this, I updated sequencer.c to use the configured commentChar for the Branch comments. I also tweaked the tests in t3430 to verify todo list generation with a custom commentChar. I'm not sure if I took the right approach with that, or if it would be better to add additional tests for that case, so feel free to tweak/replace/ignore the second commit as appropriate. Nothing is as powerful as an idea whose time has come. Or as a patch whose time has come, I guess: https://public-inbox.org/git/20180628020414.25036-1-aa...@schrab.com/ Oops, I should have done a bit a searching before I tossed off a patch. Thanks Johannes for the pointer. AFAICT the remaining task was to send a new revision of the patch, with the commit message touched up, to reflect the analysis that it handles the `auto` setting well. Your patch adds a regression test in addition, which is very nice. So maybe you can coordinate with Aaron about that first patch? I really think that the commit message needs to explain why the `auto` setting is not a problem here. Aaron, how would you like to move forward on this? I don't want to take credit from you since you were the first to post the patch. If you would like to post a new version of your patch with the commit message updated based on the feedback, I can then add my tests to go with it. Alternatively if you'd like me to run with this I can repost the patch with you as the author along with an updated commit message and my name in a "Commit-message-by:" line. Let me know your thoughts. If I don't hear from you in a couple of days, I'll go ahead and repost the patch as I described. Thanks, Daniel Harding
Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar
Hi Daniel, On Sun, 8 Jul 2018, Daniel Harding wrote: > I have core.commentChar set in my .gitconfig, and when I tried to run > git rebase -i -r, I received an error message like the following: > > error: invalid line 3: # Branch > > To fix this, I updated sequencer.c to use the configured commentChar > for the Branch comments. I also tweaked the tests in t3430 to > verify todo list generation with a custom commentChar. I'm not sure > if I took the right approach with that, or if it would be better to > add additional tests for that case, so feel free to > tweak/replace/ignore the second commit as appropriate. Nothing is as powerful as an idea whose time has come. Or as a patch whose time has come, I guess: https://public-inbox.org/git/20180628020414.25036-1-aa...@schrab.com/ AFAICT the remaining task was to send a new revision of the patch, with the commit message touched up, to reflect the analysis that it handles the `auto` setting well. Your patch adds a regression test in addition, which is very nice. So maybe you can coordinate with Aaron about that first patch? I really think that the commit message needs to explain why the `auto` setting is not a problem here. Ciao, Dscho
[PATCH 0/2] Fix --rebase-merges with custom commentChar
I have core.commentChar set in my .gitconfig, and when I tried to run git rebase -i -r, I received an error message like the following: error: invalid line 3: # Branch To fix this, I updated sequencer.c to use the configured commentChar for the Branch comments. I also tweaked the tests in t3430 to verify todo list generation with a custom commentChar. I'm not sure if I took the right approach with that, or if it would be better to add additional tests for that case, so feel free to tweak/replace/ignore the second commit as appropriate. Thanks, Daniel Harding
