[PATCH 0/2] Fix --rebase-merges with custom commentChar

[PATCH 0/2] Fix --rebase-merges with custom commentChar

[Daniel Harding]

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 <name>

To fix this, I updated sequencer.c to use the configured commentChar
for the Branch <name> 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 1/2] sequencer: fix --rebase-merges with custom commentChar

[Daniel Harding]

Prefix the "Branch <name>" comments in the todo list with the configured
comment character instead of hard-coding '#'.

Signed-off-by: Daniel Harding <dharding@living180.net>
---
 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(&state.commit2label, &commit->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 2/2] t3430: update to test with custom commentChar

[Daniel Harding]

Signed-off-by: Daniel Harding <dharding@living180.net>
---
 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

Re: [PATCH 2/2] t3430: update to test with custom commentChar

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On Sun, Jul 08, 2018 at 09:41:11PM +0300, Daniel Harding wrote:
> Signed-off-by: Daniel Harding <dharding@living180.net>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH 2/2] t3430: update to test with custom commentChar

[Johannes Schindelin]

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 <dharding@living180.net>
> 
> 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 <base> &&
	sed "s/#/%/" <todo-list.copy >expect &&
	test_config core.commentChar % &&
	git rebase -r <base> &&
	test_cmp expect todo-list.copy

Ciao,
Johannes

Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar

[Johannes Schindelin]

Hi Daniel,

On Sun, 8 Jul 2018, Daniel Harding wrote:

> I have core.commentChar set in my .gitconfig, and when I tried to run
> git rebase -i -r, I received an error message like the following:
> 
> error: invalid line 3: # Branch <name>
> 
> To fix this, I updated sequencer.c to use the configured commentChar
> for the Branch <name> 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-aaron@schrab.com/

AFAICT the remaining task was to send a new revision of the patch, with
the commit message touched up, to reflect the analysis that it handles the
`auto` setting well.

Your patch adds a regression test in addition, which is very nice.

So maybe you can coordinate with Aaron about that first patch? I really
think that the commit message needs to explain why the `auto` setting is
not a problem here.

Ciao,
Dscho

Re: [PATCH 2/2] t3430: update to test with custom commentChar

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 <dharding@living180.net>
>> 
>> 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 <base> &&
> 	sed "s/#/%/" <todo-list.copy >expect &&
> 	test_config core.commentChar % &&
> 	git rebase -r <base> &&
> 	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

[Daniel Harding]

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 <dharding@living180.net>
>>
>> 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 <base> &&
> 	sed "s/#/%/" <todo-list.copy >expect &&
> 	test_config core.commentChar % &&
> 	git rebase -r <base> &&
> 	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

[Daniel Harding]

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 <dharding@living180.net>
> 
>> 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

[Johannes Schindelin]

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

[Johannes Schindelin]

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 <dharding@living180.net>
> > 
> > > 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
<hash> <oneline>` 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

[Junio C Hamano]

Daniel Harding <dharding@living180.net> 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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH 2/2] t3430: update to test with custom commentChar

[Daniel Harding]

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
> <hash> <oneline>` 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

[Johannes Schindelin]

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
> > <hash> <oneline>` 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 0/2] Fix --rebase-merges with custom commentChar

[Daniel Harding]

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 <name>
>>
>> To fix this, I updated sequencer.c to use the configured commentChar
>> for the Branch <name> 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-aaron@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

[Daniel Harding]

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
>>> <hash> <oneline>` 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

[Aaron Schrab]

Sorry for taking so long to get back to this. Hopefully the following 
will be acceptable to everyone.

8< -----

Subject: [PATCH v2] sequencer: use configured comment character

Use the configured comment character when generating comments about
branches in a todo list.  Failure to honor this configuration causes a
failure to parse the resulting todo list.

Note that the comment_line_char has already been resolved by this point,
even if the user has configured the comment character to be selected
automatically.

Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 4034c0461b..caf91af29d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		entry = oidmap_get(&state.commit2label, &commit->object.oid);
 
 		if (entry)
-			fprintf(out, "\n# Branch %s\n", entry->string);
+			fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
 		else
 			fprintf(out, "\n");
 
-- 
2.18.0.419.gfe4b301394

Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> Subject: [PATCH v2] sequencer: use configured comment character
>
> Use the configured comment character when generating comments about
> branches in a todo list.  Failure to honor this configuration causes a
> failure to parse the resulting todo list.

OK.

>
> Note that the comment_line_char has already been resolved by this point,
> even if the user has configured the comment character to be selected
> automatically.

Isn't this a slight lie?

The core.commentchar=auto setting is noticed by everybody (including
the users of the sequencer machinery), but it is honored only by
builtin/commit.c::prepare_to_commit() that is called by
builtin/commit.c::cmd_commit(), i.e. the implementation of "git
commit" that should not be used as a subroutine by other commands,
and by nothing else.  If the user has core.commentchar=auto, the
comment_line_char is left to the default '#' in the sequencer
codepath.

I think the patch is still correct and safe, but the reason why it
is so is not because we chose a suitable character (that is how I
read what "has already been resolved by this point" means) by
calling builtin/commit.c::adjust_comment_line_char().  Isn't it
because the "script" the function is working on does not have a line
that came from arbitrary end-user input that may happen to begin
with '#', hence the default '#' is safe to use?

> Signed-off-by: Aaron Schrab <aaron@schrab.com>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 4034c0461b..caf91af29d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  		entry = oidmap_get(&state.commit2label, &commit->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");

[PATCH v3] sequencer: use configured comment character

[Aaron Schrab]

At 10:15 -0700 12 Jul 2018, Junio C Hamano <gitster@pobox.com> wrote:
>Aaron Schrab <aaron@schrab.com> writes:
>> Note that the comment_line_char has already been resolved by this point,
>> even if the user has configured the comment character to be selected
>> automatically.
>
>Isn't this a slight lie?

Looking into that a bit further, it does seem like my explanation above 
was incorrect.  Here's another attempt to explain why setting 
core.commentChar=auto isn't a problem for this change.

8< -----

Use the configured comment character when generating comments about
branches in a todo list.  Failure to honor this configuration causes a
failure to parse the resulting todo list.

Setting core.commentChar to "auto" will not be honored here, and the
previously configured or default value will be used instead. But, since
the todo list will consist of only generated content, there should not
be any non-comment lines beginning with that character.

Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 4034c0461b..caf91af29d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		entry = oidmap_get(&state.commit2label, &commit->object.oid);
 
 		if (entry)
-			fprintf(out, "\n# Branch %s\n", entry->string);
+			fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string);
 		else
 			fprintf(out, "\n");
 
-- 
2.18.0.419.gfe4b301394

Re: [PATCH v3] sequencer: use configured comment character

[Johannes Schindelin]

Hi Aaron,

On Mon, 16 Jul 2018, Aaron Schrab wrote:

> At 10:15 -0700 12 Jul 2018, Junio C Hamano <gitster@pobox.com> wrote:
> >Aaron Schrab <aaron@schrab.com> writes:
> >> Note that the comment_line_char has already been resolved by this point,
> >> even if the user has configured the comment character to be selected
> >> automatically.
> >
> >Isn't this a slight lie?
> 
> Looking into that a bit further, it does seem like my explanation above 
> was incorrect.  Here's another attempt to explain why setting 
> core.commentChar=auto isn't a problem for this change.
> 
> 8< -----
> 
> Use the configured comment character when generating comments about
> branches in a todo list.  Failure to honor this configuration causes a
> failure to parse the resulting todo list.
> 
> Setting core.commentChar to "auto" will not be honored here, and the
> previously configured or default value will be used instead. But, since
> the todo list will consist of only generated content, there should not
> be any non-comment lines beginning with that character.

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.

Ciao,
Johannes

Re: [PATCH v3] sequencer: use configured comment character

[Daniel Harding]

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 v3] sequencer: use configured comment character

[Johannes Schindelin]

Hi Daniel,

On Mon, 16 Jul 2018, Daniel Harding wrote:

> On Mon, 16 Jul 2018 at 18:59:03 +0300, Johannes Schindelin wrote:
> > 
> > On Mon, 16 Jul 2018, Aaron Schrab wrote:
> > >
> > > Looking into that a bit further, it does seem like my explanation above
> > > was incorrect.  Here's another attempt to explain why setting
> > > core.commentChar=auto isn't a problem for this change.
> > >
> > > 8< -----
> > >
> > > Use the configured comment character when generating comments about
> > > branches in a todo list.  Failure to honor this configuration causes a
> > > failure to parse the resulting todo list.
> > >
> > > Setting core.commentChar to "auto" will not be honored here, and the
> > > previously configured or default value will be used instead. But, since
> > > the todo list will consist of only generated content, there should not
> > > be any non-comment lines beginning with that character.
> > 
> > How about this instead?
> > 
> >  If core.commentChar is set to "auto", the intention is to
> >  determine the comment line character from whatever content is there
> >  already.
> > 
> >  As the code path in question is the one *generating* the todo list
> >  from scratch, it will automatically use whatever core.commentChar
> >  has been configured before the "auto" (and fall back to "#" if none
> >  has been configured explicitly), which is consistent with users'
> >  expectations.
> 
> Honestly, the above still doesn't read clearly to me.  I've take a stab at it
> myself - let me know what you think:
> 
>     If core.commentChar is set to "auto", the comment_line_char global
>     variable will be initialized to '#'.  The only time
>     comment_line_char gets changed to an automatic value is when the
>     prepare_to_commit() function (in commit.c) calls
>     adjust_comment_line_char().  This does not happen when generating
>     the todo list, so '#' will be used as the comment character in the
>     todo list if core.commentChar is set to "auto".

There is a concocted way to have core.commentChar = auto *and* to override
the comment char: if you use `git config --add core.commentChar auto`, or
if you have it set in $HOME/.gitconfig and in .git/config.

I tried to cover that in my suggestion, but that was probably trying to be
too precise, rather than being useful...

Ciao,
Johannes

Re: [PATCH 2/2] t3430: update to test with custom commentChar

[Johannes Schindelin]

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
> > > > <hash> <oneline>` 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