[PATCH] rebase -i: fix cases ignoring core.commentchar

[PATCH] rebase -i: fix cases ignoring core.commentchar

[Eric Sunshine]

eff80a9fd990de36 (Allow custom "comment char", 2013-01-16) added general
core.commentchar support but forgot to update git-rebase--interactive to
respect it.  180bad3d10fe3a7f (rebase -i: respect core.commentchar,
2013-02-11) addressed this oversight but missed one instance of
hard-coded '#' comment character in skip_unnecessary_picks(). Fix this.

9a46c25bdbf79744 (rebase: interactive: fix short SHA-1 collision,
2013-08-12) added another instance of hard-coded '#' comment character
in transform_todo_ids().  Fix this, as well.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

While planning to work on a couple patches to address Junio's comments
on [1] (currently in 'next'), I realized that that series introduced a
small regression by ignoring core.commentchar and assuming that the todo
list comment character is unconditionally '#'. This patch fixes that
regression.

While fixing the regression, I also noticed that the original patch
(180bad3d10fe3a7f) which added core.commentchar support to rebase -i
missed one instance. This patch fixes that, as well.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/232146

 git-rebase--interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ea11e62..f246810 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -671,7 +671,7 @@ skip_unnecessary_picks () {
 				;;
 			esac
 			;;
-		3,#*|3,)
+		3,"$comment_char"*|3,)
 			# copy comments
 			;;
 		*)
@@ -693,7 +693,7 @@ transform_todo_ids () {
 	while read -r command rest
 	do
 		case "$command" in
-		'#'* | exec)
+		"$comment_char"* | exec)
 			# Be careful for oddball commands like 'exec'
 			# that do not have a SHA-1 at the beginning of $rest.
 			;;
-- 
1.8.4.rc3.500.gc3113b0

Re: [PATCH] rebase -i: fix cases ignoring core.commentchar

[Eric Sunshine]

On Fri, Aug 16, 2013 at 5:44 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> eff80a9fd990de36 (Allow custom "comment char", 2013-01-16) added general
> core.commentchar support but forgot to update git-rebase--interactive to
> respect it.  180bad3d10fe3a7f (rebase -i: respect core.commentchar,
> 2013-02-11) addressed this oversight but missed one instance of
> hard-coded '#' comment character in skip_unnecessary_picks(). Fix this.
>
> 9a46c25bdbf79744 (rebase: interactive: fix short SHA-1 collision,
> 2013-08-12) added another instance of hard-coded '#' comment character
> in transform_todo_ids().  Fix this, as well.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

I forgot to mention that I wanted to add tests to t3404 for these bugs
but couldn't figure out how to do it using the external behavior of
rebase -i. I was able to verify the before and after behavior by
adding temporary echo's to the code in order to observe the "internal"
functioning.

Re: [PATCH] rebase -i: fix cases ignoring core.commentchar

[Eric Sunshine]

On Fri, Aug 16, 2013 at 5:55 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Aug 16, 2013 at 5:44 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> eff80a9fd990de36 (Allow custom "comment char", 2013-01-16) added general
>> core.commentchar support but forgot to update git-rebase--interactive to
>> respect it.  180bad3d10fe3a7f (rebase -i: respect core.commentchar,
>> 2013-02-11) addressed this oversight but missed one instance of
>> hard-coded '#' comment character in skip_unnecessary_picks(). Fix this.
>>
>> 9a46c25bdbf79744 (rebase: interactive: fix short SHA-1 collision,
>> 2013-08-12) added another instance of hard-coded '#' comment character
>> in transform_todo_ids().  Fix this, as well.
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>
> I forgot to mention that I wanted to add tests to t3404 for these bugs
> but couldn't figure out how to do it using the external behavior of
> rebase -i. I was able to verify the before and after behavior by
> adding temporary echo's to the code in order to observe the "internal"
> functioning.

One of the fixes in this patch addresses an oversight in
180bad3d10fe3a7f (rebase -i: respect core.commentchar, 2013-02-11)
which is already in 'maint'. Should I split this patch in two so that
the one fix can be applied to 'maint'?

(The other fix is for 9a46c25bdbf79744 in 'next'.)

Re: [PATCH] rebase -i: fix cases ignoring core.commentchar

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> One of the fixes in this patch addresses an oversight in
> 180bad3d10fe3a7f (rebase -i: respect core.commentchar, 2013-02-11)
> which is already in 'maint'. Should I split this patch in two so that
> the one fix can be applied to 'maint'?

We are so close to 1.8.4 final and I do not think it makes much
difference in practice, but if we were shooting for the ideal, yeah,
as that part of the patch could make core.commentchar useful for
rebase-i users who are still on 1.8.3.X maintenance track.

Re: [PATCH] rebase -i: fix cases ignoring core.commentchar

[Eric Sunshine]

On Sun, Aug 18, 2013 at 4:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> One of the fixes in this patch addresses an oversight in
>> 180bad3d10fe3a7f (rebase -i: respect core.commentchar, 2013-02-11)
>> which is already in 'maint'. Should I split this patch in two so that
>> the one fix can be applied to 'maint'?
>
> We are so close to 1.8.4 final and I do not think it makes much
> difference in practice, but if we were shooting for the ideal, yeah,
> as that part of the patch could make core.commentchar useful for
> rebase-i users who are still on 1.8.3.X maintenance track.

I sent a v2 [1] of this submission which does split the patch into two
parts: one for 'maint' and one for 'next', with appropriate commit
messages. The actual changes to git-rebase--interactive.sh are
identical between v1 and v2.

[1]: http://git.661346.n2.nabble.com/PATCH-v2-0-2-fix-cases-of-rebase-i-ignoring-core-commentchar-td7594009.html