Re: [PATCH 2/2] t3430: update to test with custom commentChar
Hi Daniel, [I forgot to address this mail earlier, my apologies] On Tue, 10 Jul 2018, Daniel Harding wrote: > 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? That is a very good idea, if you ask me! Unfortunately, I forgot about this suggestion, and IIUC v2.19.0 shipped with my previously-suggested version. But maybe you want to spend a little time on the code to change it according to your suggestion? The `merge` commands are generated here: https://github.com/git/git/blob/v2.19.0/sequencer.c#L4096-L4120 > > 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. I did want to have an extra special visual marker for users (such as myself), so a space would not quite be enough. That's why I settled for the comment char. Ciao, Johannes
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 2/2] t3430: update to test with custom commentChar
Hi Daniel, 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 `#`). 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. Ciao, Dscho
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
On Mon, Jul 09, 2018 at 09:48:55PM +0300, Daniel Harding wrote: > Hello brian, > > 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. Fair enough. Thanks for the explanation. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 2/2] t3430: update to test with custom commentChar
Daniel Harding writes: > 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? I think trusting stripspace is OK here. Even though this test is not about stripspace (i.e. trying to catch breakage in stripspace), if we broke it, we'll notice it as a side effect of running this test ;-).
Re: [PATCH 2/2] t3430: update to test with custom commentChar
Hi Daniel, On Mon, 9 Jul 2018, Daniel Harding wrote: > 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 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. Ciao, Johannes
Re: [PATCH 2/2] t3430: update to test with custom commentChar
Hi Daniel, On Mon, 9 Jul 2018, Daniel Harding wrote: > 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? I personally have no strong preference there; I tend to trust the "portability" of Git's tools more than the idiosyncrasies of whatever `grep` any setup happens to sport. But I am relatively certain that it is the exact opposite for, say, Junio (Git's maintainer), so... Ciao, Johannes
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
Re: [PATCH 2/2] t3430: update to test with custom commentChar
Johannes Schindelin writes: > 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 > > Ciao, > Johannes Sounds sensible. Nice to see multiple brains working together going in the right direction ;-)
Re: [PATCH 2/2] t3430: update to test with custom commentChar
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 Ciao, Johannes
Re: [PATCH 2/2] t3430: update to test with custom commentChar
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/#/>/'". > 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. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
[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
