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
Re: [PATCH 2/2] t3430: update to test with custom commentChar
Hi Johannes, On Tue, 10 Jul 2018 at 16:08:57 +0300, Johannes Schindelin wrote:> On Tue, 10 Jul 2018, Daniel Harding wrote: On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote: On Mon, 9 Jul 2018, Daniel Harding wrote: On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote: Should this affect the "# Merge the topic branch" line (and the "# C", "# E", and "# H" lines in the next test) that appears below this? It would seem those would qualify as comments as well. I intentionally did not change that behavior for two reasons: a) from a Git perspective, comment characters are only effectual for comments if they are the first character in a line and b) there are places where a '#' character from the todo list is actually parsed and used e.g. [0] and [1]. I have not yet gotten to the point of grokking what is going on there, so I didn't want to risk breaking something I didn't understand. Perhaps Johannes could shed some light on whether the cases you mentioned should be changed to use the configured commentChar or not. [0] https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869 [1] https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797 These are related. The first one tries to support merge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch i.e. use '#' to separate between the commit(s) to merge and the oneline (the latter for the reader's pleasure, just like the onelines in the `pick ` lines. The second ensures that there is no valid label `#`. I have not really thought about the ramifications of changing this to comment_line_char, but I guess it *could* work if both locations were changed. Is there interest in such a change? I'm happy to take a stab at it if there is, otherwise I'll leave things as they are. I think it would be a fine change, once we convinced ourselves that it does not break things (I am a little worried about this because I remember just how long I had to reflect about the ramifications with regards to the label: `#` is a valid ref name, after all, and that was the reason why I had to treat it specially, and I wonder whether allowing arbitrary comment chars will require us to add more such special handling that is not necessary if we stick to `#`). Would it simpler/safer to perhaps put the oneline on its own commented line above? I know it isn't quite consistent with the way onelines are displayed for normal commits, but it might be a worthwhile tradeoff for the sake of the code. As an idea of what I am suggesting, your example above would become perhaps # Merge: Octopus 2nd/3rd branch merge -C cafecafe second-branch third-branch or perhaps just # Octopus 2nd/3rd branch merge -C cafecafe second-branch third-branch Thoughts? Not that the comment line char feature seems to be all that safe. I could imagine that setting it to ' ' (i.e. a single space) wreaks havoc with Git, and we have no safeguard to error out in this obviously broken case. Technically, I think a single space might actually work with commit messages (at least, I can't off the top of my head think of a case where git would insert a non-comment line starting with a space if it wasn't already present in a commit message). But if someone were actually crazy enough to do that I might suggest a diagnosis of "if it hurts, don't do that" rather than trying to equip git defend against that sort of thing. Daniel Harding
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 2/2] t3430: update to test with custom commentChar
On Mon, 09 Jul 2018 at 22:14:58 +0300, Johannes Schindelin wrote: On Mon, 9 Jul 2018, Daniel Harding wrote: On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote: >>> Should this affect the "# Merge the topic branch" line (and the "# C", "# E", and "# H" lines in the next test) that appears below this? It would seem those would qualify as comments as well. I intentionally did not change that behavior for two reasons: a) from a Git perspective, comment characters are only effectual for comments if they are the first character in a line and b) there are places where a '#' character from the todo list is actually parsed and used e.g. [0] and [1]. I have not yet gotten to the point of grokking what is going on there, so I didn't want to risk breaking something I didn't understand. Perhaps Johannes could shed some light on whether the cases you mentioned should be changed to use the configured commentChar or not. [0] https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869 [1] https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797 These are related. The first one tries to support merge -C cafecafe second-branch third-branch # Octopus 2nd/3rd branch i.e. use '#' to separate between the commit(s) to merge and the oneline (the latter for the reader's pleasure, just like the onelines in the `pick ` lines. The second ensures that there is no valid label `#`. I have not really thought about the ramifications of changing this to comment_line_char, but I guess it *could* work if both locations were changed. Is there interest in such a change? I'm happy to take a stab at it if there is, otherwise I'll leave things as they are. Daniel Harding
Re: [PATCH 2/2] t3430: update to test with custom commentChar
Hello brian, On Mon, 09 Jul 2018 at 00:02:00 +0300, brian m. carlson wrote: On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote: Signed-off-by: Daniel Harding diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 78f7c9958..ff474d033 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -56,12 +56,12 @@ test_expect_success 'create completely different structure' ' cat >script-from-scratch <<-\EOF && label onto - # onebranch + > onebranch pick G pick D label onebranch - # second + > second reset onto pick B label second Should this affect the "# Merge the topic branch" line (and the "# C", "# E", and "# H" lines in the next test) that appears below this? It would seem those would qualify as comments as well. I intentionally did not change that behavior for two reasons: a) from a Git perspective, comment characters are only effectual for comments if they are the first character in a line and b) there are places where a '#' character from the todo list is actually parsed and used e.g. [0] and [1]. I have not yet gotten to the point of grokking what is going on there, so I didn't want to risk breaking something I didn't understand. Perhaps Johannes could shed some light on whether the cases you mentioned should be changed to use the configured commentChar or not. [0] https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L2869 [1] https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/sequencer.c#L3797 Thanks, Daniel Harding
Re: [PATCH 2/2] t3430: update to test with custom commentChar
Hi Johannes, On Mon, 09 Jul 2018 at 10:52:13 +0300, Johannes Schindelin wrote: Hi Brian, On Sun, 8 Jul 2018, brian m. carlson wrote: On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote: Signed-off-by: Daniel Harding I think maybe, as you suggested, a separate test for this would be beneficial. It might be as simple as modifying 'script-from-scratch' by doing "sed 's/#/>/'". It might be even simpler if you come up with a new "fake editor" to merely copy the todo list, then run a rebase without overridden commentChar, then one with overridden commentChar, then pipe the todo list of the first through that `sed` call: write_script copy-todo-list.sh <<-\EOF && cp "$1" todo-list.copy EOF test_config sequence.editor \""$PWD"/copy-todo-list.sh\" && git rebase -r && sed "s/#/%/" expect && test_config core.commentChar % && git rebase -r && test_cmp expect todo-list.copy Indeed, as I thought about it more, using a "no-op" todo editor seemed like a good approach. Thanks for giving me a head start - I'll play with that and try to get a new patch with an improved test posted in the next couple of days. One question about my original patch - there I had replaced a "grep -v" call with a "git stripspace" call in the 'generate correct todo list' test. Is relying on "git stripspace" in a test acceptable, or should external text manipulation tools like grep, sed etc. be preferred? Thanks, Daniel Harding
[PATCH 1/2] sequencer: fix --rebase-merges with custom commentChar
Prefix the "Branch " comments in the todo list with the configured comment character instead of hard-coding '#'. Signed-off-by: Daniel Harding --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 4034c0461..caf91af29 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
[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
[PATCH 2/2] t3430: update to test with custom commentChar
Signed-off-by: Daniel Harding --- t/t3430-rebase-merges.sh | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 78f7c9958..ff474d033 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -56,12 +56,12 @@ test_expect_success 'create completely different structure' ' cat >script-from-scratch <<-\EOF && label onto - # onebranch + > onebranch pick G pick D label onebranch - # second + > second reset onto pick B label second @@ -70,6 +70,7 @@ test_expect_success 'create completely different structure' ' merge -C H second merge onebranch # Merge the topic branch '\''onebranch'\'' EOF + test_config core.commentChar ">" && test_config sequence.editor \""$PWD"/replace-editor.sh\" && test_tick && git rebase -i -r A && @@ -107,10 +108,10 @@ test_expect_success 'generate correct todo list' ' pick 12bd07b D merge -C 2051b56 E # E merge -C 233d48a H # H - EOF - grep -v "^#" <.git/ORIGINAL-TODO >output && + test_config core.commentChar ">" && + git stripspace -s <.git/ORIGINAL-TODO >output && test_cmp expect output ' -- 2.18.0
