threads / patch / 34715

patchrebase -i: fix cases ignoring core.commentchar

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

## tl;dr

5 messages between Aug 16, 2013 and Aug 18, 2013. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Eric Sunshine· Aug 16, 2013, 21:44 UTC · lore

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(-)
Show changes to git-rebase--interactive.sh +2 −2
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
Eric Sunshine· Aug 16, 2013, 21:55 UTC · re: Eric Sunshine · lore

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

On Fri, Aug 16, 2013 at 5:44 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 11 quoted lines
> 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.

Eric Sunshine· Aug 16, 2013, 22:19 UTC · re: Eric Sunshine · lore

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

On Fri, Aug 16, 2013 at 5:55 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 18 quoted lines
> 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'.)
Junio C Hamano· Aug 18, 2013, 20:29 UTC · re: Eric Sunshine · lore

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

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.

Eric Sunshine· Aug 18, 2013, 20:36 UTC · re: Junio C Hamano · lore

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

On Sun, Aug 18, 2013 at 4:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> 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

← back to recent threads