{"thread":{"id":"48841","subject":"[PATCH 0/2] Fix --rebase-merges with custom commentChar","startedAt":"2018-07-08T18:51:24Z","lastAt":"2018-10-02T14:39:08Z","messageCount":24,"participants":["Daniel Harding","brian m. carlson","Johannes Schindelin","Junio C Hamano","Aaron Schrab"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"351884","messageId":"20180708184110.14792-1-dharding@living180.net","threadId":"48841","inReplyTo":null,"subject":"[PATCH 0/2] Fix --rebase-merges with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-08T18:41:08Z","receivedAt":"2018-07-08T18:51:24Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"I have core.commentChar set in my .gitconfig, and when I tried to run\ngit rebase -i -r, I received an error message like the following:\n\nerror: invalid line 3: # Branch \n\nTo fix this, I updated sequencer.c to use the configured commentChar\nfor the Branch comments. I also tweaked the tests in t3430 to\nverify todo list generation with a custom commentChar. I'm not sure\nif I took the right approach with that, or if it would be better to\nadd additional tests for that case, so feel free to\ntweak/replace/ignore the second commit as appropriate.\n\nThanks,\n\nDaniel Harding\n\n\n"},{"id":"351885","messageId":"20180708184110.14792-2-dharding@living180.net","threadId":"48841","inReplyTo":"20180708184110.14792-1-dharding@living180.net","subject":"[PATCH 1/2] sequencer: fix --rebase-merges with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-08T18:41:10Z","receivedAt":"2018-07-08T18:51:26Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"Prefix the \"Branch \" comments in the todo list with the configured\ncomment character instead of hard-coding '#'.\n\nSigned-off-by: Daniel Harding \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4034c0461..caf91af29 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\tentry = oidmap_get(&state.commit2label, &commit->object.oid);\n \n \t\tif (entry)\n-\t\t\tfprintf(out, \"\\n# Branch %s\\n\", entry->string);\n+\t\t\tfprintf(out, \"\\n%c Branch %s\\n\", comment_line_char, entry->string);\n \t\telse\n \t\t\tfprintf(out, \"\\n\");\n \n-- \n2.18.0\n\n"},{"id":"351886","messageId":"20180708184110.14792-3-dharding@living180.net","threadId":"48841","inReplyTo":"20180708184110.14792-1-dharding@living180.net","subject":"[PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-08T18:41:11Z","receivedAt":"2018-07-08T18:51:28Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"Signed-off-by: Daniel Harding \n---\n t/t3430-rebase-merges.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 78f7c9958..ff474d033 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -56,12 +56,12 @@ test_expect_success 'create completely different structure' '\n \tcat >script-from-scratch <<-\\EOF &&\n \tlabel onto\n \n-\t# onebranch\n+\t> onebranch\n \tpick G\n \tpick D\n \tlabel onebranch\n \n-\t# second\n+\t> second\n \treset onto\n \tpick B\n \tlabel second\n@@ -70,6 +70,7 @@ test_expect_success 'create completely different structure' '\n \tmerge -C H second\n \tmerge onebranch # Merge the topic branch '\\''onebranch'\\''\n \tEOF\n+\ttest_config core.commentChar \">\" &&\n \ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n \ttest_tick &&\n \tgit rebase -i -r A &&\n@@ -107,10 +108,10 @@ test_expect_success 'generate correct todo list' '\n \tpick 12bd07b D\n \tmerge -C 2051b56 E # E\n \tmerge -C 233d48a H # H\n-\n \tEOF\n \n-\tgrep -v \"^#\" <.git/ORIGINAL-TODO >output &&\n+\ttest_config core.commentChar \">\" &&\n+\tgit stripspace -s <.git/ORIGINAL-TODO >output &&\n \ttest_cmp expect output\n '\n \n-- \n2.18.0\n\n"},{"id":"351889","messageId":"20180708210200.GA4573@genre.crustytoothpaste.net","threadId":"48841","inReplyTo":"20180708184110.14792-3-dharding@living180.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-08T21:02:00Z","receivedAt":"2018-07-08T21:02:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:\n> Signed-off-by: Daniel Harding \n\nI think maybe, as you suggested, a separate test for this would be\nbeneficial. It might be as simple as modifying 'script-from-scratch' by\ndoing \"sed 's/#/>/'\".\n\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index 78f7c9958..ff474d033 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -56,12 +56,12 @@ test_expect_success 'create completely different structure' '\n> \tcat >script-from-scratch <<-\\EOF &&\n> \tlabel onto\n> \n> -\t# onebranch\n> +\t> onebranch\n> \tpick G\n> \tpick D\n> \tlabel onebranch\n> \n> -\t# second\n> +\t> second\n> \treset onto\n> \tpick B\n> \tlabel second\n\nShould this affect the \"# Merge the topic branch\" line (and the \"# C\",\n\"# E\", and \"# H\" lines in the next test) that appears below this? It\nwould seem those would qualify as comments as well.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"351925","messageId":"nycvar.QRO.7.76.6.1807090944400.75@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"20180708210200.GA4573@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-09T07:52:13Z","receivedAt":"2018-07-09T07:52:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn Sun, 8 Jul 2018, brian m. carlson wrote:\n\n> On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:\n> > Signed-off-by: Daniel Harding \n> \n> I think maybe, as you suggested, a separate test for this would be\n> beneficial. It might be as simple as modifying 'script-from-scratch' by\n> doing \"sed 's/#/>/'\".\n\nIt might be even simpler if you come up with a new \"fake editor\" to merely\ncopy the todo list, then run a rebase without overridden\ncommentChar, then one with overridden commentChar, then pipe the todo list\nof the first through that `sed` call:\n\n\n write_script copy-todo-list.sh <<-\\EOF &&\n cp \"$1\" todo-list.copy\n EOF\n\ttest_config sequence.editor \\\"\"$PWD\"/copy-todo-list.sh\\\" &&\n\tgit rebase -r &&\n\tsed \"s/#/%/\" expect &&\n\ttest_config core.commentChar % &&\n\tgit rebase -r &&\n\ttest_cmp expect todo-list.copy\n\nCiao,\nJohannes\n"},{"id":"351926","messageId":"nycvar.QRO.7.76.6.1807090936230.75@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"20180708184110.14792-1-dharding@living180.net","subject":"Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-09T07:53:14Z","receivedAt":"2018-07-09T07:53:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Sun, 8 Jul 2018, Daniel Harding wrote:\n\n> I have core.commentChar set in my .gitconfig, and when I tried to run\n> git rebase -i -r, I received an error message like the following:\n> \n> error: invalid line 3: # Branch \n> \n> To fix this, I updated sequencer.c to use the configured commentChar\n> for the Branch comments. I also tweaked the tests in t3430 to\n> verify todo list generation with a custom commentChar. I'm not sure\n> if I took the right approach with that, or if it would be better to\n> add additional tests for that case, so feel free to\n> tweak/replace/ignore the second commit as appropriate.\n\nNothing is as powerful as an idea whose time has come. Or as a patch whose\ntime has come, I guess:\n\nhttps://public-inbox.org/git/20180628020414.25036-1-aaron@schrab.com/\n\nAFAICT the remaining task was to send a new revision of the patch, with\nthe commit message touched up, to reflect the analysis that it handles the\n`auto` setting well.\n\nYour patch adds a regression test in addition, which is very nice.\n\nSo maybe you can coordinate with Aaron about that first patch? I really\nthink that the commit message needs to explain why the `auto` setting is\nnot a problem here.\n\nCiao,\nDscho\n"},{"id":"351956","messageId":"xmqq4lh8fjcc.fsf@gitster-ct.c.googlers.com","threadId":"48841","inReplyTo":"nycvar.QRO.7.76.6.1807090944400.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T16:46:11Z","receivedAt":"2018-07-09T16:46:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Brian,\n>\n> On Sun, 8 Jul 2018, brian m. carlson wrote:\n>\n>> On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:\n>> > Signed-off-by: Daniel Harding \n>> \n>> I think maybe, as you suggested, a separate test for this would be\n>> beneficial. It might be as simple as modifying 'script-from-scratch' by\n>> doing \"sed 's/#/>/'\".\n>\n> It might be even simpler if you come up with a new \"fake editor\" to merely\n> copy the todo list, then run a rebase without overridden\n> commentChar, then one with overridden commentChar, then pipe the todo list\n> of the first through that `sed` call:\n>\n>\n> write_script copy-todo-list.sh <<-\\EOF &&\n> cp \"$1\" todo-list.copy\n> EOF\n> \ttest_config sequence.editor \\\"\"$PWD\"/copy-todo-list.sh\\\" &&\n> \tgit rebase -r &&\n> \tsed \"s/#/%/\" expect &&\n> \ttest_config core.commentChar % &&\n> \tgit rebase -r &&\n> \ttest_cmp expect todo-list.copy\n>\n> Ciao,\n> Johannes\n\nSounds sensible. Nice to see multiple brains working together going\nin the right direction ;-)\n"},{"id":"351975","messageId":"13a876a2-7fbc-de05-2e82-814c782e8a80@living180.net","threadId":"48841","inReplyTo":"nycvar.QRO.7.76.6.1807090944400.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-09T18:22:22Z","receivedAt":"2018-07-09T18:22:31Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"Hi Johannes,\n\nOn Mon, 09 Jul 2018 at 10:52:13 +0300, Johannes Schindelin wrote:\n> Hi Brian,\n> \n> On Sun, 8 Jul 2018, brian m. carlson wrote:\n> \n>> On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:\n>>> Signed-off-by: Daniel Harding \n>>\n>> I think maybe, as you suggested, a separate test for this would be\n>> beneficial. It might be as simple as modifying 'script-from-scratch' by\n>> doing \"sed 's/#/>/'\".\n> \n> It might be even simpler if you come up with a new \"fake editor\" to merely\n> copy the todo list, then run a rebase without overridden\n> commentChar, then one with overridden commentChar, then pipe the todo list\n> of the first through that `sed` call:\n> \n> \n> write_script copy-todo-list.sh <<-\\EOF &&\n> cp \"$1\" todo-list.copy\n> EOF\n> \ttest_config sequence.editor \\\"\"$PWD\"/copy-todo-list.sh\\\" &&\n> \tgit rebase -r &&\n> \tsed \"s/#/%/\" expect &&\n> \ttest_config core.commentChar % &&\n> \tgit rebase -r &&\n> \ttest_cmp expect todo-list.copy\n\nIndeed, as I thought about it more, using a \"no-op\" todo editor seemed \nlike a good approach. Thanks for giving me a head start - I'll play \nwith that and try to get a new patch with an improved test posted in the \nnext couple of days.\n\nOne question about my original patch - there I had replaced a \"grep -v\" \ncall with a \"git stripspace\" call in the 'generate correct todo list' \ntest. Is relying on \"git stripspace\" in a test acceptable, or should \nexternal text manipulation tools like grep, sed etc. be preferred?\n\nThanks,\n\nDaniel Harding\n"},{"id":"351979","messageId":"1084a573-4ed5-5a8c-a159-7773f7465704@living180.net","threadId":"48841","inReplyTo":"20180708210200.GA4573@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-09T18:48:55Z","receivedAt":"2018-07-09T18:49:03Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"Hello brian,\n\nOn Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:\n> On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:\n>> Signed-off-by: Daniel Harding \n> \n>> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n>> index 78f7c9958..ff474d033 100755\n>> --- a/t/t3430-rebase-merges.sh\n>> +++ b/t/t3430-rebase-merges.sh\n>> @@ -56,12 +56,12 @@ test_expect_success 'create completely different structure' '\n>> \tcat >script-from-scratch <<-\\EOF &&\n>> \tlabel onto\n>> \n>> -\t# onebranch\n>> +\t> onebranch\n>> \tpick G\n>> \tpick D\n>> \tlabel onebranch\n>> \n>> -\t# second\n>> +\t> second\n>> \treset onto\n>> \tpick B\n>> \tlabel second\n> \n> Should this affect the \"# Merge the topic branch\" line (and the \"# C\",\n> \"# E\", and \"# H\" lines in the next test) that appears below this? It\n> would seem those would qualify as comments as well.\n\nI intentionally did not change that behavior for two reasons:\n\na) from a Git perspective, comment characters are only effectual for \ncomments if they are the first character in a line\n\nand\n\nb) there are places where a '#' character from the todo list is actually \nparsed and used e.g. [0] and [1]. I have not yet gotten to the point of \ngrokking what is going on there, so I didn't want to risk breaking \nsomething I didn't understand. Perhaps Johannes could shed some light \non whether the cases you mentioned should be changed to use the \nconfigured commentChar or not.\n\n[0] \nhttps://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869\n[1] \nhttps://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797\n\nThanks,\n\nDaniel Harding\n"},{"id":"351981","messageId":"nycvar.QRO.7.76.6.1807092107260.75@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"13a876a2-7fbc-de05-2e82-814c782e8a80@living180.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-09T19:09:34Z","receivedAt":"2018-07-09T19:09:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Mon, 9 Jul 2018, Daniel Harding wrote:\n\n> One question about my original patch - there I had replaced a \"grep -v\"\n> call with a \"git stripspace\" call in the 'generate correct todo list'\n> test. Is relying on \"git stripspace\" in a test acceptable, or should\n> external text manipulation tools like grep, sed etc. be preferred?\n\nI personally have no strong preference there; I tend to trust the\n\"portability\" of Git's tools more than the idiosyncrasies of whatever\n`grep` any setup happens to sport. But I am relatively certain that it is\nthe exact opposite for, say, Junio (Git's maintainer), so...\n\nCiao,\nJohannes\n"},{"id":"351982","messageId":"nycvar.QRO.7.76.6.1807092109440.75@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"1084a573-4ed5-5a8c-a159-7773f7465704@living180.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-09T19:14:58Z","receivedAt":"2018-07-09T19:15:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Mon, 9 Jul 2018, Daniel Harding wrote:\n\n> On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:\n> > On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:\n> > > Signed-off-by: Daniel Harding \n> > \n> > > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> > > index 78f7c9958..ff474d033 100755\n> > > --- a/t/t3430-rebase-merges.sh\n> > > +++ b/t/t3430-rebase-merges.sh\n> > > @@ -56,12 +56,12 @@ test_expect_success 'create completely different\n> > > structure' '\n> > > cat >script-from-scratch <<-\\EOF &&\n> > > label onto\n> > > -\t# onebranch\n> > > +\t > onebranch\n> > > pick G\n> > > pick D\n> > > label onebranch\n> > > -\t# second\n> > > +\t > second\n> > > reset onto\n> > > pick B\n> > > label second\n> > \n> > Should this affect the \"# Merge the topic branch\" line (and the \"# C\",\n> > \"# E\", and \"# H\" lines in the next test) that appears below this? It\n> > would seem those would qualify as comments as well.\n> \n> I intentionally did not change that behavior for two reasons:\n> \n> a) from a Git perspective, comment characters are only effectual for comments\n> if they are the first character in a line\n> \n> and\n> \n> b) there are places where a '#' character from the todo list is actually\n> parsed and used e.g. [0] and [1]. I have not yet gotten to the point of\n> grokking what is going on there, so I didn't want to risk breaking something I\n> didn't understand. Perhaps Johannes could shed some light on whether the\n> cases you mentioned should be changed to use the configured commentChar or\n> not.\n> \n> [0]\n> https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869\n> [1]\n> https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797\n\nThese are related. The first one tries to support\n\n\tmerge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch\n\ni.e. use '#' to separate between the commit(s) to merge and the oneline\n(the latter for the reader's pleasure, just like the onelines in the `pick\n ` lines.\n\nThe second ensures that there is no valid label `#`.\n\nI have not really thought about the ramifications of changing this to\ncomment_line_char, but I guess it *could* work if both locations were\nchanged.\n\nCiao,\nJohannes\n"},{"id":"352003","messageId":"xmqqpnzwcgyi.fsf@gitster-ct.c.googlers.com","threadId":"48841","inReplyTo":"13a876a2-7fbc-de05-2e82-814c782e8a80@living180.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T20:05:57Z","receivedAt":"2018-07-09T20:06:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Harding writes:\n\n> One question about my original patch - there I had replaced a \"grep\n> -v\" call with a \"git stripspace\" call in the 'generate correct todo\n> list' test. Is relying on \"git stripspace\" in a test acceptable, or\n> should external text manipulation tools like grep, sed etc. be\n> preferred?\n\nI think trusting stripspace is OK here. Even though this test is\nnot about stripspace (i.e. trying to catch breakage in stripspace),\nif we broke it, we'll notice it as a side effect of running this\ntest ;-).\n"},{"id":"352050","messageId":"20180709234149.GC535220@genre.crustytoothpaste.net","threadId":"48841","inReplyTo":"1084a573-4ed5-5a8c-a159-7773f7465704@living180.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-09T23:41:49Z","receivedAt":"2018-07-09T23:41:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Jul 09, 2018 at 09:48:55PM +0300, Daniel Harding wrote:\n> Hello brian,\n> \n> On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:\n> > Should this affect the \"# Merge the topic branch\" line (and the \"# C\",\n> > \"# E\", and \"# H\" lines in the next test) that appears below this? It\n> > would seem those would qualify as comments as well.\n> \n> I intentionally did not change that behavior for two reasons:\n> \n> a) from a Git perspective, comment characters are only effectual for\n> comments if they are the first character in a line\n> \n> and\n> \n> b) there are places where a '#' character from the todo list is actually\n> parsed and used e.g. [0] and [1]. I have not yet gotten to the point of\n> grokking what is going on there, so I didn't want to risk breaking something\n> I didn't understand. Perhaps Johannes could shed some light on whether the\n> cases you mentioned should be changed to use the configured commentChar or\n> not.\n\nFair enough. Thanks for the explanation.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"352106","messageId":"aa716d3f-6a80-e3fc-0172-1027fb85c792@living180.net","threadId":"48841","inReplyTo":"nycvar.QRO.7.76.6.1807092109440.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-10T12:29:51Z","receivedAt":"2018-07-10T12:29:59Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote:\n> \n> On Mon, 9 Jul 2018, Daniel Harding wrote:\n>> \n>> On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:\n >>>\n>>> Should this affect the \"# Merge the topic branch\" line (and the \"# C\",\n>>> \"# E\", and \"# H\" lines in the next test) that appears below this? It\n>>> would seem those would qualify as comments as well.\n>>\n>> I intentionally did not change that behavior for two reasons:\n>>\n>> a) from a Git perspective, comment characters are only effectual for comments\n>> if they are the first character in a line\n>>\n>> and\n>>\n>> b) there are places where a '#' character from the todo list is actually\n>> parsed and used e.g. [0] and [1]. I have not yet gotten to the point of\n>> grokking what is going on there, so I didn't want to risk breaking something I\n>> didn't understand. Perhaps Johannes could shed some light on whether the\n>> cases you mentioned should be changed to use the configured commentChar or\n>> not.\n>>\n>> [0]\n>> https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869\n>> [1]\n>> https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797\n> \n> These are related. The first one tries to support\n> \n> \tmerge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch\n> \n> i.e. use '#' to separate between the commit(s) to merge and the oneline\n> (the latter for the reader's pleasure, just like the onelines in the `pick\n> ` lines.\n> \n> The second ensures that there is no valid label `#`.\n> \n> I have not really thought about the ramifications of changing this to\n> comment_line_char, but I guess it *could* work if both locations were\n> changed.\n\nIs there interest in such a change? I'm happy to take a stab at it if \nthere is, otherwise I'll leave things as they are.\n\nDaniel Harding\n"},{"id":"352107","messageId":"nycvar.QRO.7.76.6.1807101504530.75@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"aa716d3f-6a80-e3fc-0172-1027fb85c792@living180.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-10T13:08:57Z","receivedAt":"2018-07-10T13:09:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Tue, 10 Jul 2018, Daniel Harding wrote:\n\n> On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote:\n> > \n> > On Mon, 9 Jul 2018, Daniel Harding wrote:\n> > > \n> > > On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:\n> > > >\n> > > > Should this affect the \"# Merge the topic branch\" line (and the \"# C\",\n> > > > \"# E\", and \"# H\" lines in the next test) that appears below this? It\n> > > > would seem those would qualify as comments as well.\n> > >\n> > > I intentionally did not change that behavior for two reasons:\n> > >\n> > > a) from a Git perspective, comment characters are only effectual for\n> > > comments\n> > > if they are the first character in a line\n> > >\n> > > and\n> > >\n> > > b) there are places where a '#' character from the todo list is actually\n> > > parsed and used e.g. [0] and [1]. I have not yet gotten to the point of\n> > > grokking what is going on there, so I didn't want to risk breaking\n> > > something I\n> > > didn't understand. Perhaps Johannes could shed some light on whether the\n> > > cases you mentioned should be changed to use the configured commentChar or\n> > > not.\n> > >\n> > > [0]\n> > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869\n> > > [1]\n> > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797\n> > \n> > These are related. The first one tries to support\n> > \n> > merge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch\n> > \n> > i.e. use '#' to separate between the commit(s) to merge and the oneline\n> > (the latter for the reader's pleasure, just like the onelines in the `pick\n> > ` lines.\n> > \n> > The second ensures that there is no valid label `#`.\n> > \n> > I have not really thought about the ramifications of changing this to\n> > comment_line_char, but I guess it *could* work if both locations were\n> > changed.\n> \n> Is there interest in such a change? I'm happy to take a stab at it if there\n> is, otherwise I'll leave things as they are.\n\nI think it would be a fine change, once we convinced ourselves that it\ndoes not break things (I am a little worried about this because I remember\njust how long I had to reflect about the ramifications with regards to the\nlabel: `#` is a valid ref name, after all, and that was the reason why I\nhad to treat it specially, and I wonder whether allowing arbitrary comment\nchars will require us to add more such special handling that is not\nnecessary if we stick to `#`).\n\nNot that the comment line char feature seems to be all that safe. I could\nimagine that setting it to ' ' (i.e. a single space) wreaks havoc with\nGit, and we have no safeguard to error out in this obviously broken case.\n\nCiao,\nDscho\n"},{"id":"352109","messageId":"e8973797-fc5f-2ca5-1881-5ee66fc8279b@living180.net","threadId":"48841","inReplyTo":"nycvar.QRO.7.76.6.1807090936230.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-10T13:24:19Z","receivedAt":"2018-07-10T13:24:27Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"On Mon, 09 Jul 2018 at 10:53:14 +0300, Johannes Schindelin wrote>\n> On Sun, 8 Jul 2018, Daniel Harding wrote:\n> \n>> I have core.commentChar set in my .gitconfig, and when I tried to run\n>> git rebase -i -r, I received an error message like the following:\n>>\n>> error: invalid line 3: # Branch \n>>\n>> To fix this, I updated sequencer.c to use the configured commentChar\n>> for the Branch comments. I also tweaked the tests in t3430 to\n>> verify todo list generation with a custom commentChar. I'm not sure\n>> if I took the right approach with that, or if it would be better to\n>> add additional tests for that case, so feel free to\n>> tweak/replace/ignore the second commit as appropriate.\n> \n> Nothing is as powerful as an idea whose time has come. Or as a patch whose\n> time has come, I guess:\n> \n> https://public-inbox.org/git/20180628020414.25036-1-aaron@schrab.com/\n\nOops, I should have done a bit a searching before I tossed off a patch. \nThanks Johannes for the pointer.\n\n> AFAICT the remaining task was to send a new revision of the patch, with\n> the commit message touched up, to reflect the analysis that it handles the\n> `auto` setting well.\n> \n> Your patch adds a regression test in addition, which is very nice.\n> \n> So maybe you can coordinate with Aaron about that first patch? I really\n> think that the commit message needs to explain why the `auto` setting is\n> not a problem here.\n\nAaron, how would you like to move forward on this? I don't want to take \ncredit from you since you were the first to post the patch. If you \nwould like to post a new version of your patch with the commit message \nupdated based on the feedback, I can then add my tests to go with it. \nAlternatively if you'd like me to run with this I can repost the patch \nwith you as the author along with an updated commit message and my name \nin a \"Commit-message-by:\" line. Let me know your thoughts. If I don't \nhear from you in a couple of days, I'll go ahead and repost the patch as \nI described.\n\nThanks,\n\nDaniel Harding\n"},{"id":"352110","messageId":"9628abb4-86ec-a6f1-0c9c-64458949ebdb@living180.net","threadId":"48841","inReplyTo":"nycvar.QRO.7.76.6.1807101504530.75@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-10T13:49:14Z","receivedAt":"2018-07-10T13:49:22Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"Hi Johannes,\n\nOn Tue, 10 Jul 2018 at 16:08:57 +0300, Johannes Schindelin wrote:>\n> On Tue, 10 Jul 2018, Daniel Harding wrote:\n> \n>> On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote:\n>>>\n>>> On Mon, 9 Jul 2018, Daniel Harding wrote:\n>>>>\n>>>> On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:\n>>>>>\n>>>>> Should this affect the \"# Merge the topic branch\" line (and the \"# C\",\n>>>>> \"# E\", and \"# H\" lines in the next test) that appears below this? It\n>>>>> would seem those would qualify as comments as well.\n>>>>\n>>>> I intentionally did not change that behavior for two reasons:\n>>>>\n>>>> a) from a Git perspective, comment characters are only effectual for\n>>>> comments\n>>>> if they are the first character in a line\n>>>>\n>>>> and\n>>>>\n>>>> b) there are places where a '#' character from the todo list is actually\n>>>> parsed and used e.g. [0] and [1]. I have not yet gotten to the point of\n>>>> grokking what is going on there, so I didn't want to risk breaking\n>>>> something I\n>>>> didn't understand. Perhaps Johannes could shed some light on whether the\n>>>> cases you mentioned should be changed to use the configured commentChar or\n>>>> not.\n>>>>\n>>>> [0]\n>>>> https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869\n>>>> [1]\n>>>> https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797\n>>>\n>>> These are related. The first one tries to support\n>>>\n>>> merge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch\n>>>\n>>> i.e. use '#' to separate between the commit(s) to merge and the oneline\n>>> (the latter for the reader's pleasure, just like the onelines in the `pick\n>>> ` lines.\n>>>\n>>> The second ensures that there is no valid label `#`.\n>>>\n>>> I have not really thought about the ramifications of changing this to\n>>> comment_line_char, but I guess it *could* work if both locations were\n>>> changed.\n>>\n>> Is there interest in such a change? I'm happy to take a stab at it if there\n>> is, otherwise I'll leave things as they are.\n> \n> I think it would be a fine change, once we convinced ourselves that it\n> does not break things (I am a little worried about this because I remember\n> just how long I had to reflect about the ramifications with regards to the\n> label: `#` is a valid ref name, after all, and that was the reason why I\n> had to treat it specially, and I wonder whether allowing arbitrary comment\n> chars will require us to add more such special handling that is not\n> necessary if we stick to `#`).\n\nWould it simpler/safer to perhaps put the oneline on its own commented \nline above? I know it isn't quite consistent with the way onelines are \ndisplayed for normal commits, but it might be a worthwhile tradeoff for \nthe sake of the code. As an idea of what I am suggesting, your example \nabove would become perhaps\n\n # Merge: Octopus 2nd/3rd branch\n merge -C cafecafe second-branch third-branch\n\nor perhaps just\n\n # Octopus 2nd/3rd branch\n merge -C cafecafe second-branch third-branch\n\nThoughts?\n\n> Not that the comment line char feature seems to be all that safe. I could\n> imagine that setting it to ' ' (i.e. a single space) wreaks havoc with\n> Git, and we have no safeguard to error out in this obviously broken case.\n\nTechnically, I think a single space might actually work with commit \nmessages (at least, I can't off the top of my head think of a case where \ngit would insert a non-comment line starting with a space if it wasn't \nalready present in a commit message). But if someone were actually \ncrazy enough to do that I might suggest a diagnosis of \"if it hurts, \ndon't do that\" rather than trying to equip git defend against that sort \nof thing.\n\nDaniel Harding\n"},{"id":"352320","messageId":"20180712030249.22071-1-aaron@schrab.com","threadId":"48841","inReplyTo":"e8973797-fc5f-2ca5-1881-5ee66fc8279b@living180.net","subject":"Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2018-07-12T03:02:49Z","receivedAt":"2018-07-12T03:03:48Z","isPatch":true,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"Sorry for taking so long to get back to this. Hopefully the following \nwill be acceptable to everyone.\n\n8< -----\n\nSubject: [PATCH v2] sequencer: use configured comment character\n\nUse the configured comment character when generating comments about\nbranches in a todo list. Failure to honor this configuration causes a\nfailure to parse the resulting todo list.\n\nNote that the comment_line_char has already been resolved by this point,\neven if the user has configured the comment character to be selected\nautomatically.\n\nSigned-off-by: Aaron Schrab \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4034c0461b..caf91af29d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\tentry = oidmap_get(&state.commit2label, &commit->object.oid);\n \n \t\tif (entry)\n-\t\t\tfprintf(out, \"\\n# Branch %s\\n\", entry->string);\n+\t\t\tfprintf(out, \"\\n%c Branch %s\\n\", comment_line_char, entry->string);\n \t\telse\n \t\t\tfprintf(out, \"\\n\");\n \n-- \n2.18.0.419.gfe4b301394\n\n"},{"id":"352369","messageId":"xmqq4lh4z870.fsf@gitster-ct.c.googlers.com","threadId":"48841","inReplyTo":"20180712030249.22071-1-aaron@schrab.com","subject":"Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-12T17:15:47Z","receivedAt":"2018-07-12T17:15:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Schrab writes:\n\n> Subject: [PATCH v2] sequencer: use configured comment character\n>\n> Use the configured comment character when generating comments about\n> branches in a todo list. Failure to honor this configuration causes a\n> failure to parse the resulting todo list.\n\nOK.\n\n>\n> Note that the comment_line_char has already been resolved by this point,\n> even if the user has configured the comment character to be selected\n> automatically.\n\nIsn't this a slight lie?\n\nThe core.commentchar=auto setting is noticed by everybody (including\nthe users of the sequencer machinery), but it is honored only by\nbuiltin/commit.c::prepare_to_commit() that is called by\nbuiltin/commit.c::cmd_commit(), i.e. the implementation of \"git\ncommit\" that should not be used as a subroutine by other commands,\nand by nothing else. If the user has core.commentchar=auto, the\ncomment_line_char is left to the default '#' in the sequencer\ncodepath.\n\nI think the patch is still correct and safe, but the reason why it\nis so is not because we chose a suitable character (that is how I\nread what \"has already been resolved by this point\" means) by\ncalling builtin/commit.c::adjust_comment_line_char(). Isn't it\nbecause the \"script\" the function is working on does not have a line\nthat came from arbitrary end-user input that may happen to begin\nwith '#', hence the default '#' is safe to use?\n\n> Signed-off-by: Aaron Schrab \n> ---\n> sequencer.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 4034c0461b..caf91af29d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n> \t\tentry = oidmap_get(&state.commit2label, &commit->object.oid);\n> \n> \t\tif (entry)\n> -\t\t\tfprintf(out, \"\\n# Branch %s\\n\", entry->string);\n> +\t\t\tfprintf(out, \"\\n%c Branch %s\\n\", comment_line_char, entry->string);\n> \t\telse\n> \t\t\tfprintf(out, \"\\n\");\n"},{"id":"352599","messageId":"20180716045902.16629-1-aaron@schrab.com","threadId":"48841","inReplyTo":"xmqq4lh4z870.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v3] sequencer: use configured comment character","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2018-07-16T04:59:02Z","receivedAt":"2018-07-16T04:59:17Z","isPatch":true,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"At 10:15 -0700 12 Jul 2018, Junio C Hamano wrote:\n>Aaron Schrab writes:\n>> Note that the comment_line_char has already been resolved by this point,\n>> even if the user has configured the comment character to be selected\n>> automatically.\n>\n>Isn't this a slight lie?\n\nLooking into that a bit further, it does seem like my explanation above \nwas incorrect. Here's another attempt to explain why setting \ncore.commentChar=auto isn't a problem for this change.\n\n8< -----\n\nUse the configured comment character when generating comments about\nbranches in a todo list. Failure to honor this configuration causes a\nfailure to parse the resulting todo list.\n\nSetting core.commentChar to \"auto\" will not be honored here, and the\npreviously configured or default value will be used instead. But, since\nthe todo list will consist of only generated content, there should not\nbe any non-comment lines beginning with that character.\n\nSigned-off-by: Aaron Schrab \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4034c0461b..caf91af29d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \t\tentry = oidmap_get(&state.commit2label, &commit->object.oid);\n \n \t\tif (entry)\n-\t\t\tfprintf(out, \"\\n# Branch %s\\n\", entry->string);\n+\t\t\tfprintf(out, \"\\n%c Branch %s\\n\", comment_line_char, entry->string);\n \t\telse\n \t\t\tfprintf(out, \"\\n\");\n \n-- \n2.18.0.419.gfe4b301394\n\n"},{"id":"352632","messageId":"nycvar.QRO.7.76.6.1807161758560.71@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"20180716045902.16629-1-aaron@schrab.com","subject":"Re: [PATCH v3] sequencer: use configured comment character","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-16T15:59:03Z","receivedAt":"2018-07-16T15:59:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Aaron,\n\nOn Mon, 16 Jul 2018, Aaron Schrab wrote:\n\n> At 10:15 -0700 12 Jul 2018, Junio C Hamano wrote:\n> >Aaron Schrab writes:\n> >> Note that the comment_line_char has already been resolved by this point,\n> >> even if the user has configured the comment character to be selected\n> >> automatically.\n> >\n> >Isn't this a slight lie?\n> \n> Looking into that a bit further, it does seem like my explanation above \n> was incorrect. Here's another attempt to explain why setting \n> core.commentChar=auto isn't a problem for this change.\n> \n> 8< -----\n> \n> Use the configured comment character when generating comments about\n> branches in a todo list. Failure to honor this configuration causes a\n> failure to parse the resulting todo list.\n> \n> Setting core.commentChar to \"auto\" will not be honored here, and the\n> previously configured or default value will be used instead. But, since\n> the todo list will consist of only generated content, there should not\n> be any non-comment lines beginning with that character.\n\nHow about this instead?\n\n\tIf core.commentChar is set to \"auto\", the intention is to\n\tdetermine the comment line character from whatever content is there\n\talready.\n\n\tAs the code path in question is the one *generating* the todo list\n\tfrom scratch, it will automatically use whatever core.commentChar\n\thas been configured before the \"auto\" (and fall back to \"#\" if none\n\thas been configured explicitly), which is consistent with users'\n\texpectations.\n\nCiao,\nJohannes\n"},{"id":"352649","messageId":"66ce6f94-49ae-618e-bf6c-43a0f15bb752@living180.net","threadId":"48841","inReplyTo":"nycvar.QRO.7.76.6.1807161758560.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] sequencer: use configured comment character","fromName":"Daniel Harding","fromEmail":"dharding@living180.net","sentAt":"2018-07-16T18:49:58Z","receivedAt":"2018-07-16T18:50:04Z","isPatch":true,"sender":{"key":"dharding@living180.net","avatar":"https://gravatar.com/avatar/b6c0035ef883248945371388b5229124b15c8accf9ff20768f43e7c4df560a56?d=mp&s=160"},"body":"Hi Johannes,\n\nOn Mon, 16 Jul 2018 at 18:59:03 +0300, Johannes Schindelin wrote:\n> Hi Aaron,\n> \n> On Mon, 16 Jul 2018, Aaron Schrab wrote:\n>>\n>> Looking into that a bit further, it does seem like my explanation above\n>> was incorrect. Here's another attempt to explain why setting\n>> core.commentChar=auto isn't a problem for this change.\n>>\n>> 8< -----\n>>\n>> Use the configured comment character when generating comments about\n>> branches in a todo list. Failure to honor this configuration causes a\n>> failure to parse the resulting todo list.\n>>\n>> Setting core.commentChar to \"auto\" will not be honored here, and the\n>> previously configured or default value will be used instead. But, since\n>> the todo list will consist of only generated content, there should not\n>> be any non-comment lines beginning with that character.\n> \n> How about this instead?\n> \n> \tIf core.commentChar is set to \"auto\", the intention is to\n> \tdetermine the comment line character from whatever content is there\n> \talready.\n> \n> \tAs the code path in question is the one *generating* the todo list\n> \tfrom scratch, it will automatically use whatever core.commentChar\n> \thas been configured before the \"auto\" (and fall back to \"#\" if none\n> \thas been configured explicitly), which is consistent with users'\n> \texpectations.\n\nHonestly, the above still doesn't read clearly to me. I've take a stab \nat it myself - let me know what you think:\n\n If core.commentChar is set to \"auto\", the comment_line_char global\n variable will be initialized to '#'. The only time\n comment_line_char gets changed to an automatic value is when the\n prepare_to_commit() function (in commit.c) calls\n adjust_comment_line_char(). This does not happen when generating\n the todo list, so '#' will be used as the comment character in the\n todo list if core.commentChar is set to \"auto\".\n\nCheers,\n\nDaniel Harding\n"},{"id":"352796","messageId":"nycvar.QRO.7.76.6.1807171844040.71@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"66ce6f94-49ae-618e-bf6c-43a0f15bb752@living180.net","subject":"Re: [PATCH v3] sequencer: use configured comment character","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-17T16:46:03Z","receivedAt":"2018-07-17T16:46:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\nOn Mon, 16 Jul 2018, Daniel Harding wrote:\n\n> On Mon, 16 Jul 2018 at 18:59:03 +0300, Johannes Schindelin wrote:\n> > \n> > On Mon, 16 Jul 2018, Aaron Schrab wrote:\n> > >\n> > > Looking into that a bit further, it does seem like my explanation above\n> > > was incorrect. Here's another attempt to explain why setting\n> > > core.commentChar=auto isn't a problem for this change.\n> > >\n> > > 8< -----\n> > >\n> > > Use the configured comment character when generating comments about\n> > > branches in a todo list. Failure to honor this configuration causes a\n> > > failure to parse the resulting todo list.\n> > >\n> > > Setting core.commentChar to \"auto\" will not be honored here, and the\n> > > previously configured or default value will be used instead. But, since\n> > > the todo list will consist of only generated content, there should not\n> > > be any non-comment lines beginning with that character.\n> > \n> > How about this instead?\n> > \n> > If core.commentChar is set to \"auto\", the intention is to\n> > determine the comment line character from whatever content is there\n> > already.\n> > \n> > As the code path in question is the one *generating* the todo list\n> > from scratch, it will automatically use whatever core.commentChar\n> > has been configured before the \"auto\" (and fall back to \"#\" if none\n> > has been configured explicitly), which is consistent with users'\n> > expectations.\n> \n> Honestly, the above still doesn't read clearly to me. I've take a stab at it\n> myself - let me know what you think:\n> \n> If core.commentChar is set to \"auto\", the comment_line_char global\n> variable will be initialized to '#'. The only time\n> comment_line_char gets changed to an automatic value is when the\n> prepare_to_commit() function (in commit.c) calls\n> adjust_comment_line_char(). This does not happen when generating\n> the todo list, so '#' will be used as the comment character in the\n> todo list if core.commentChar is set to \"auto\".\n\nThere is a concocted way to have core.commentChar = auto *and* to override\nthe comment char: if you use `git config --add core.commentChar auto`, or\nif you have it set in $HOME/.gitconfig and in .git/config.\n\nI tried to cover that in my suggestion, but that was probably trying to be\ntoo precise, rather than being useful...\n\nCiao,\nJohannes\n"},{"id":"359419","messageId":"nycvar.QRO.7.76.6.1810021629020.2034@tvgsbejvaqbjf.bet","threadId":"48841","inReplyTo":"9628abb4-86ec-a6f1-0c9c-64458949ebdb@living180.net","subject":"Re: [PATCH 2/2] t3430: update to test with custom commentChar","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-02T14:38:54Z","receivedAt":"2018-10-02T14:39:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Daniel,\n\n[I forgot to address this mail earlier, my apologies]\n\nOn Tue, 10 Jul 2018, Daniel Harding wrote:\n\n> On Tue, 10 Jul 2018 at 16:08:57 +0300, Johannes Schindelin wrote:>\n> > On Tue, 10 Jul 2018, Daniel Harding wrote:\n> > \n> > > On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote:\n> > > >\n> > > > On Mon, 9 Jul 2018, Daniel Harding wrote:\n> > > > >\n> > > > > On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote:\n> > > > > >\n> > > > > > Should this affect the \"# Merge the topic branch\" line (and the \"#\n> > > > > > C\",\n> > > > > > \"# E\", and \"# H\" lines in the next test) that appears below this?\n> > > > > > It\n> > > > > > would seem those would qualify as comments as well.\n> > > > >\n> > > > > I intentionally did not change that behavior for two reasons:\n> > > > >\n> > > > > a) from a Git perspective, comment characters are only effectual for\n> > > > > comments\n> > > > > if they are the first character in a line\n> > > > >\n> > > > > and\n> > > > >\n> > > > > b) there are places where a '#' character from the todo list is\n> > > > > actually\n> > > > > parsed and used e.g. [0] and [1]. I have not yet gotten to the point\n> > > > > of\n> > > > > grokking what is going on there, so I didn't want to risk breaking\n> > > > > something I\n> > > > > didn't understand. Perhaps Johannes could shed some light on whether\n> > > > > the\n> > > > > cases you mentioned should be changed to use the configured\n> > > > > commentChar or\n> > > > > not.\n> > > > >\n> > > > > [0]\n> > > > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869\n> > > > > [1]\n> > > > > https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797\n> > > >\n> > > > These are related. The first one tries to support\n> > > >\n> > > > merge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch\n> > > >\n> > > > i.e. use '#' to separate between the commit(s) to merge and the oneline\n> > > > (the latter for the reader's pleasure, just like the onelines in the\n> > > > `pick\n> > > > ` lines.\n> > > >\n> > > > The second ensures that there is no valid label `#`.\n> > > >\n> > > > I have not really thought about the ramifications of changing this to\n> > > > comment_line_char, but I guess it *could* work if both locations were\n> > > > changed.\n> > >\n> > > Is there interest in such a change? I'm happy to take a stab at it if\n> > > there\n> > > is, otherwise I'll leave things as they are.\n> > \n> > I think it would be a fine change, once we convinced ourselves that it\n> > does not break things (I am a little worried about this because I remember\n> > just how long I had to reflect about the ramifications with regards to the\n> > label: `#` is a valid ref name, after all, and that was the reason why I\n> > had to treat it specially, and I wonder whether allowing arbitrary comment\n> > chars will require us to add more such special handling that is not\n> > necessary if we stick to `#`).\n> \n> Would it simpler/safer to perhaps put the oneline on its own commented line\n> above? I know it isn't quite consistent with the way onelines are displayed\n> for normal commits, but it might be a worthwhile tradeoff for the sake of the\n> code. As an idea of what I am suggesting, your example above would become\n> perhaps\n> \n> # Merge: Octopus 2nd/3rd branch\n> merge -C cafecafe second-branch third-branch\n> \n> or perhaps just\n> \n> # Octopus 2nd/3rd branch\n> merge -C cafecafe second-branch third-branch\n> \n> Thoughts?\n\nThat is a very good idea, if you ask me! Unfortunately, I forgot about\nthis suggestion, and IIUC v2.19.0 shipped with my previously-suggested\nversion.\n\nBut maybe you want to spend a little time on the code to change it\naccording to your suggestion?\n\nThe `merge` commands are generated here:\nhttps://github.com/git/git/blob/v2.19.0/sequencer.c#L4096-L4120\n\n> > Not that the comment line char feature seems to be all that safe. I could\n> > imagine that setting it to ' ' (i.e. a single space) wreaks havoc with\n> > Git, and we have no safeguard to error out in this obviously broken case.\n> \n> Technically, I think a single space might actually work with commit messages\n> (at least, I can't off the top of my head think of a case where git would\n> insert a non-comment line starting with a space if it wasn't already present\n> in a commit message). But if someone were actually crazy enough to do that I\n> might suggest a diagnosis of \"if it hurts, don't do that\" rather than trying\n> to equip git defend against that sort of thing.\n\nI did want to have an extra special visual marker for users (such as\nmyself), so a space would not quite be enough. That's why I settled for\nthe comment char.\n\nCiao,\nJohannes\n"}]}