{"thread":{"id":"40456","subject":"[PATCH] rebase: accept indented comments (fixes regression)","startedAt":"2015-09-30T08:11:01Z","lastAt":"2015-10-01T08:18:42Z","messageCount":13,"participants":["Matthieu Moy","Remi Galan Alfonso","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270928","messageId":"1443600661-19391-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"40456","inReplyTo":null,"subject":"[PATCH] rebase: accept indented comments (fixes regression)","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-09-30T08:11:01Z","receivedAt":"2015-09-30T08:11:01Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"With Git <2.0.6, 'git rebase' used to accept lines starting with\nwhitespaces followed with '#' as a comment. This was broken by\n804098b (git rebase -i: add static check for commands and SHA-1,\n2015-06-29), which introduced additional checks on the TODO-list using\n\"git stripspaces\" which only strips comments starting at the first\ncolumn.\n\nWhether it's a good thing to accept indented comments is\ndebatable (other commands like \"git commit\" do not accept them), but we\nalready accepted them in the past, and some people and scripts rely on\nthis behavior. Also, a line starting with space followed by a '#' cannot\nhave any meaning other than being a comment, hence it doesn't harm to\naccept them as comments.\n\nSigned-off-by: Matthieu Moy \n---\nJunio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> I know you alluded to preprocess what is fed to stripspace, but I\n>> wonder if we can remove the misguided call to stripspace in the\n>> first place and do something like the attached instead.\n>>\n>> git-rebase--interactive.sh | 3 +--\n>> 1 file changed, 1 insertion(+), 2 deletions(-)\n>>\n>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n>> index f01637b..a64f77a 100644\n>> --- a/git-rebase--interactive.sh\n>> +++ b/git-rebase--interactive.sh\n>> @@ -886,7 +886,6 @@ check_commit_sha () {\n>> # from the todolist in stdin\n>> check_bad_cmd_and_sha () {\n>> \tretval=0\n>> -\tgit stripspace --strip-comments |\n>> \t(\n>> \t\twhile read -r line\n>> \t\tdo\n>> @@ -896,7 +895,7 @@ check_bad_cmd_and_sha () {\n>> \t\t\tsha1=$2\n>> \n>> \t\t\tcase $command in\n>> -\t\t\t''|noop|x|\"exec\")\n>> +\t\t\t'#'*|''|noop|x|\"exec\")\n>> \t\t\t\t# Doesn't expect a SHA-1\n>> \t\t\t\t;;\n>> \t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n>\n> Nah, that would not work, as I misread the \"split only at SP\" manual\n> parsing of $line.\n\nOK, let's go for the solution I seem to be able to get right even with\nlow cafeine ;-).\n\n git-rebase--interactive.sh | 3 +++\n t/t3404-rebase-interactive.sh | 10 ++++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f01637b..55adf78 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -886,6 +886,9 @@ check_commit_sha () {\n # from the todolist in stdin\n check_bad_cmd_and_sha () {\n \tretval=0\n+\t# git rebase -i accepts comments preceeded by spaces, while\n+\t# stripspace does not.\n+\tsed 's/^[[:space:]]*//' |\n \tgit stripspace --strip-comments |\n \t(\n \t\twhile read -r line\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d26e3f5..ac5bac3 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1227,6 +1227,16 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'indented comments are accepted' '\n+\trebase_setup_and_clean indented-comment &&\n+\twrite_script add-indent.sh <<-\\EOF &&\n+\tprintf \"\\n \\t # comment\\n\" >>$1\n+\tEOF\n+\ttest_set_editor \"$(pwd)/add-indent.sh\" &&\n+\tgit rebase -i HEAD^ &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n cat >expect < writes:\n> With Git <2.0.6, 'git rebase' used to accept lines starting with\n> whitespaces followed with '#' as a comment. This was broken by\n> 804098b (git rebase -i: add static check for commands and SHA-1,\n> 2015-06-29), which introduced additional checks on the TODO-list using\n> \"git stripspaces\" which only strips comments starting at the first\n> column.\n>\n> Whether it's a good thing to accept indented comments is\n> debatable (other commands like \"git commit\" do not accept them), but we\n> already accepted them in the past, and some people and scripts rely on\n> this behavior. Also, a line starting with space followed by a '#' cannot\n> have any meaning other than being a comment, hence it doesn't harm to\n> accept them as comments.\n>\n> Signed-off-by: Matthieu Moy \n\nThank you for the patch, and sorry for the introduced regression.\n\nRémi\n\n> ---\n> Junio C Hamano writes:\n>\n> > Junio C Hamano writes:\n> >\n> >> I know you alluded to preprocess what is fed to stripspace, but I\n> >> wonder if we can remove the misguided call to stripspace in the\n> >> first place and do something like the attached instead.\n> >>\n> >> git-rebase--interactive.sh | 3 +--\n> >> 1 file changed, 1 insertion(+), 2 deletions(-)\n> >>\n> >> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> >> index f01637b..a64f77a 100644\n> >> --- a/git-rebase--interactive.sh\n> >> +++ b/git-rebase--interactive.sh\n> >> @@ -886,7 +886,6 @@ check_commit_sha () {\n> >> # from the todolist in stdin\n> >> check_bad_cmd_and_sha () {\n> >> retval=0\n> >> - git stripspace --strip-comments |\n> >> (\n> >> while read -r line\n> >> do\n> >> @@ -896,7 +895,7 @@ check_bad_cmd_and_sha () {\n> >> sha1=$2\n> >>\n> >> case $command in\n> >> - ''|noop|x|\"exec\")\n> >> + '#'*|''|noop|x|\"exec\")\n> >> # Doesn't expect a SHA-1\n> >> ;;\n> >> pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n> >\n> > Nah, that would not work, as I misread the \"split only at SP\" manual\n> > parsing of $line.\n>\n> OK, let's go for the solution I seem to be able to get right even with\n> low cafeine ;-).\n>\n> git-rebase--interactive.sh | 3 +++\n> t/t3404-rebase-interactive.sh | 10 ++++++++++\n> 2 files changed, 13 insertions(+)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index f01637b..55adf78 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -886,6 +886,9 @@ check_commit_sha () {\n> # from the todolist in stdin\n> check_bad_cmd_and_sha () {\n> retval=0\n> + # git rebase -i accepts comments preceeded by spaces, while\n> + # stripspace does not.\n> + sed 's/^[[:space:]]*//' |\n> git stripspace --strip-comments |\n> (\n> while read -r line\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index d26e3f5..ac5bac3 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1227,6 +1227,16 @@ test_expect_success 'static check of bad command' '\n> test C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n> '\n>\n> +test_expect_success 'indented comments are accepted' '\n> + rebase_setup_and_clean indented-comment &&\n> + write_script add-indent.sh <<-\\EOF &&\n> + printf \"\\n \\t # comment\\n\" >>$1\n> + EOF\n> + test_set_editor \"$(pwd)/add-indent.sh\" &&\n> + git rebase -i HEAD^ &&\n> + test E = $(git cat-file commit HEAD | sed -ne \\$p)\n> +'\n> +\n> cat >expect < Warning: the SHA-1 is missing or isn't a commit in the following line:\n> - edit XXXXXXX False commit\n> --\n> 2.6.0.rc2.24.g231a9a1.dirty\n"},{"id":"270936","messageId":"xmqqd1wz9402.fsf@gitster.mtv.corp.google.com","threadId":"40456","inReplyTo":"1443600661-19391-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] rebase: accept indented comments (fixes regression)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-30T18:05:49Z","receivedAt":"2015-09-30T18:05:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> With Git <2.0.6, 'git rebase' used to accept lines starting with\n> whitespaces followed with '#' as a comment. This was broken by\n> 804098b (git rebase -i: add static check for commands and SHA-1,\n> 2015-06-29), which introduced additional checks on the TODO-list using\n> \"git stripspaces\" which only strips comments starting at the first\n> column.\n\nI cannot help thinking that this is sidestepping the real issue.\n\nThe real issue, I think, is that the new check tokenises the input\ndifferently from how the expand_todo_ids -> transform_todo_ids\ncallchain parses it. The problem Nazri Ramliy noticed about the new\ncheck that does not ignore the indentation is merely one aspect of\nit.\n\nStripping leading whilespaces with sed may ignore indented anything\nand help Nazri's script, but 804098b tightened checks to disallow\nother things that we historically allowed, e.g. if you replace SP\nbetween \"pick\" and the commit object name with an HT, the new check\nwill not notice that HT is also a perfectly good token separator\ncharacter and barfs.\n\nI am actually tempted to say that we should revert 804098b, which is\nthe simplest fix.\n\nIf we want \"check everything before doing a single thing\" mode, the\nright way to do it would be to base the check on the same loop as\ntransform_todo_ids (one way to do so would be to give a third mode\nto that helper function, but I do not think we mind a small code\nduplication).\n\nAs far as I can tell, the hand-rolled parsing is there only in oder\nto report the incoming $line as-is. It is much easier to just\nidentify with which line number the location of the problem, and\nshow it when it is necessary from the original source, and we do not\ncare about performance in the error codepath.\n\nPerhaps something along these lines instead, with your new tests\nadded in?\n\n git-rebase--interactive.sh | 62 ++++++++++++++++++++++------------------------\n 1 file changed, 29 insertions(+), 33 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex dcc3401..ae1806a 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -849,7 +849,8 @@ add_exec_commands () {\n # Check if the SHA-1 passed as an argument is a\n # correct one, if not then print $2 in \"$todo\".badsha\n # $1: the SHA-1 to test\n-# $2: the line to display if incorrect SHA-1\n+# $2: the line number of the input\n+# $3: the input filename\n check_commit_sha () {\n \tbadsha=0\n \tif test -z $1\n@@ -865,9 +866,10 @@ check_commit_sha () {\n \n \tif test $badsha -ne 0\n \tthen\n+\t\tline=\"$(sed -n -e \"${2}p\" \"$3\")\"\n \t\twarn \"Warning: the SHA-1 is missing or isn't\" \\\n \t\t\t\"a commit in the following line:\"\n-\t\twarn \" - $2\"\n+\t\twarn \" - $line\"\n \t\twarn\n \tfi\n \n@@ -878,37 +880,31 @@ check_commit_sha () {\n # from the todolist in stdin\n check_bad_cmd_and_sha () {\n \tretval=0\n-\tgit stripspace --strip-comments |\n-\t(\n-\t\twhile read -r line\n-\t\tdo\n-\t\t\tIFS=' '\n-\t\t\tset -- $line\n-\t\t\tcommand=$1\n-\t\t\tsha1=$2\n-\n-\t\t\tcase $command in\n-\t\t\t''|noop|x|\"exec\")\n-\t\t\t\t# Doesn't expect a SHA-1\n-\t\t\t\t;;\n-\t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n-\t\t\t\tif ! check_commit_sha $sha1 \"$line\"\n-\t\t\t\tthen\n-\t\t\t\t\tretval=1\n-\t\t\t\tfi\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\twarn \"Warning: the command isn't recognized\" \\\n-\t\t\t\t\t\"in the following line:\"\n-\t\t\t\twarn \" - $line\"\n-\t\t\t\twarn\n+\tlineno=0\n+\twhile read -r command rest\n+\tdo\n+\t\tlineno=$(( $lineno + 1 ))\n+\t\tcase $command in\n+\t\t\"$comment_char\"*|''|noop|x|exec)\n+\t\t\t# Doesn't expect a SHA-1\n+\t\t\t;;\n+\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\tif ! check_commit_sha \"${rest%% *}\" \"$lineno\" \"$1\"\n+\t\t\tthen\n \t\t\t\tretval=1\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\tdone\n-\n-\t\treturn $retval\n-\t)\n+\t\t\tfi\n+\t\t\t;;\n+\t\t*)\n+\t\t\tline=\"$(sed -n -e \"${lineno}p\" \"$1\")\"\n+\t\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\t\"in the following line:\"\n+\t\t\twarn \" - $line\"\n+\t\t\twarn\n+\t\t\tretval=1\n+\t\t\t;;\n+\t\tesac\n+\tdone <\"$1\"\n+\treturn $retval\n }\n \n # Print the list of the SHA-1 of the commits\n@@ -1002,7 +998,7 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n-\tif ! check_bad_cmd_and_sha <\"$todo\"\n+\tif ! check_bad_cmd_and_sha \"$todo\"\n \tthen\n \t\traise_error=t\n \tfi\n"},{"id":"270943","messageId":"vpqwpv7hg41.fsf@grenoble-inp.fr","threadId":"40456","inReplyTo":"xmqqd1wz9402.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase: accept indented comments (fixes regression)","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-30T19:17:02Z","receivedAt":"2015-09-30T19:17:02Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> With Git <2.0.6, 'git rebase' used to accept lines starting with\n>> whitespaces followed with '#' as a comment. This was broken by\n>> 804098b (git rebase -i: add static check for commands and SHA-1,\n>> 2015-06-29), which introduced additional checks on the TODO-list using\n>> \"git stripspaces\" which only strips comments starting at the first\n>> column.\n>\n> I cannot help thinking that this is sidestepping the real issue.\n>\n> The real issue, I think, is that the new check tokenises the input\n> differently from how the expand_todo_ids -> transform_todo_ids\n> callchain parses it. The problem Nazri Ramliy noticed about the new\n> check that does not ignore the indentation is merely one aspect of\n> it.\n\nRight.\n\n> Stripping leading whilespaces with sed may ignore indented anything\n> and help Nazri's script, but 804098b tightened checks to disallow\n> other things that we historically allowed, e.g. if you replace SP\n> between \"pick\" and the commit object name with an HT, the new check\n> will not notice that HT is also a perfectly good token separator\n> character and barfs.\n\nIndeed. I'm adding a test for that too.\n\n> I am actually tempted to say that we should revert 804098b, which is\n> the simplest fix.\n\nI think the commit has value, and reverting it makes the \"drop\" command\nessentially useless.\n\n> As far as I can tell, the hand-rolled parsing is there only in oder\n> to report the incoming $line as-is.\n\nIndeed, I remember finding the parsing code weird when I reviewed it,\nand the reason was to provide the exact line.\n\n> It is much easier to just identify with which line number the location\n> of the problem, and show it when it is necessary from the original\n> source, and we do not care about performance in the error codepath.\n\nAgreed.\n\n> Perhaps something along these lines instead, with your new tests\n> added in?\n\nSounds good, yes. I'll send a patch with this and my updated tests.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"270944","messageId":"xmqqy4fn7m2s.fsf@gitster.mtv.corp.google.com","threadId":"40456","inReplyTo":"xmqqd1wz9402.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase: accept indented comments (fixes regression)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-30T19:18:19Z","receivedAt":"2015-09-30T19:18:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am actually tempted to say that we should revert 804098b, which is\n> the simplest fix.\n>\n> If we want \"check everything before doing a single thing\" mode, the\n> right way to do it would be to base the check on the same loop as\n> transform_todo_ids (one way to do so would be to give a third mode\n> to that helper function, but I do not think we mind a small code\n> duplication).\n> ...\n\nSo here is a reroll, which is now minimally tested.\n\n-- >8 --\nSubject: [PATCH] rebase-i: loosen over-eager check_bad_cmd check\n\n804098bb (git rebase -i: add static check for commands and SHA-1,\n2015-06-29) tried to check all insns before running any in the todo\nlist, but it did so by implementing its own parser that is a lot\nstricter than necessary. We used to allow lines that are indented\n(including comment lines), and we used to allow a whitespace between\nthe insn and the commit object name to be HT, among other things,\nthat are flagged as an invalid line by mistake.\n\nFix this by using the same tokenizer that is used to parse the todo\nlist file in the new check. A new test is by Matthieu Moy.\n\nSigned-off-by: Junio C Hamano \n---\n git-rebase--interactive.sh | 62 ++++++++++++++++++++-----------------------\n t/t3404-rebase-interactive.sh | 10 +++++++\n 2 files changed, 39 insertions(+), 33 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex dcc3401..ae1806a 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -849,7 +849,8 @@ add_exec_commands () {\n # Check if the SHA-1 passed as an argument is a\n # correct one, if not then print $2 in \"$todo\".badsha\n # $1: the SHA-1 to test\n-# $2: the line to display if incorrect SHA-1\n+# $2: the line number of the input\n+# $3: the input filename\n check_commit_sha () {\n \tbadsha=0\n \tif test -z $1\n@@ -865,9 +866,10 @@ check_commit_sha () {\n \n \tif test $badsha -ne 0\n \tthen\n+\t\tline=\"$(sed -n -e \"${2}p\" \"$3\")\"\n \t\twarn \"Warning: the SHA-1 is missing or isn't\" \\\n \t\t\t\"a commit in the following line:\"\n-\t\twarn \" - $2\"\n+\t\twarn \" - $line\"\n \t\twarn\n \tfi\n \n@@ -878,37 +880,31 @@ check_commit_sha () {\n # from the todolist in stdin\n check_bad_cmd_and_sha () {\n \tretval=0\n-\tgit stripspace --strip-comments |\n-\t(\n-\t\twhile read -r line\n-\t\tdo\n-\t\t\tIFS=' '\n-\t\t\tset -- $line\n-\t\t\tcommand=$1\n-\t\t\tsha1=$2\n-\n-\t\t\tcase $command in\n-\t\t\t''|noop|x|\"exec\")\n-\t\t\t\t# Doesn't expect a SHA-1\n-\t\t\t\t;;\n-\t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n-\t\t\t\tif ! check_commit_sha $sha1 \"$line\"\n-\t\t\t\tthen\n-\t\t\t\t\tretval=1\n-\t\t\t\tfi\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\twarn \"Warning: the command isn't recognized\" \\\n-\t\t\t\t\t\"in the following line:\"\n-\t\t\t\twarn \" - $line\"\n-\t\t\t\twarn\n+\tlineno=0\n+\twhile read -r command rest\n+\tdo\n+\t\tlineno=$(( $lineno + 1 ))\n+\t\tcase $command in\n+\t\t\"$comment_char\"*|''|noop|x|exec)\n+\t\t\t# Doesn't expect a SHA-1\n+\t\t\t;;\n+\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\tif ! check_commit_sha \"${rest%% *}\" \"$lineno\" \"$1\"\n+\t\t\tthen\n \t\t\t\tretval=1\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\tdone\n-\n-\t\treturn $retval\n-\t)\n+\t\t\tfi\n+\t\t\t;;\n+\t\t*)\n+\t\t\tline=\"$(sed -n -e \"${lineno}p\" \"$1\")\"\n+\t\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\t\"in the following line:\"\n+\t\t\twarn \" - $line\"\n+\t\t\twarn\n+\t\t\tretval=1\n+\t\t\t;;\n+\t\tesac\n+\tdone <\"$1\"\n+\treturn $retval\n }\n \n # Print the list of the SHA-1 of the commits\n@@ -1002,7 +998,7 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n-\tif ! check_bad_cmd_and_sha <\"$todo\"\n+\tif ! check_bad_cmd_and_sha \"$todo\"\n \tthen\n \t\traise_error=t\n \tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ebdab4b..2437a3c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1206,6 +1206,16 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'indented comments are accepted' '\n+\trebase_setup_and_clean indented-comment &&\n+\twrite_script add-indent.sh <<-\\EOF &&\n+\tprintf \"\\n \\t # comment\\n\" >>$1\n+\tEOF\n+\ttest_set_editor \"$(pwd)/add-indent.sh\" &&\n+\tgit rebase -i HEAD^ &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n cat >expect < writes:\n\n> Sounds good, yes. I'll send a patch with this and my updated tests.\n\nThanks. I think our mails crossed, so I'd discard my copy that\nlacks the new test you wrote.\n"},{"id":"270951","messageId":"vpqh9mbhek2.fsf@grenoble-inp.fr","threadId":"40456","inReplyTo":"xmqqy4fn7m2s.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase: accept indented comments (fixes regression)","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-30T19:50:37Z","receivedAt":"2015-09-30T19:50:37Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> +\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n> +\t\t\tif ! check_commit_sha \"${rest%% *}\" \"$lineno\" \"$1\"\n\nThis does not pass my \"tabs\" test, as it parses the sha1 out of the line\nassuming it's separated with a space. It's used in other places of the\ncode, but tabs still seem to work more or less by chance (they are not\nparsed properly by transform_todo_ids, but then they are understood by\ndo_next).\n\nI changed it to\n\n\twhile read -r command sha1 rest\n\nwhich is a bit more lazy.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"270953","messageId":"1443643292-14505-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"40456","inReplyTo":"xmqqy4fn7m2s.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] rebase-i: loosen over-eager check_bad_cmd check","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-09-30T20:01:32Z","receivedAt":"2015-09-30T20:01:32Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"804098bb (git rebase -i: add static check for commands and SHA-1,\n2015-06-29) tried to check all insns before running any in the todo\nlist, but it did so by implementing its own parser that is a lot\nstricter than necessary. We used to allow lines that are indented\n(including comment lines), and we used to allow a whitespace between\nthe insn and the commit object name to be HT, among other things,\nthat are flagged as an invalid line by mistake.\n\nFix this by using the same tokenizer that is used to parse the todo\nlist file in the new check.\n\nWhether it's a good thing to accept indented comments is\ndebatable (other commands like \"git commit\" do not accept them), but we\nalready accepted them in the past, and some people and scripts rely on\nthis behavior. Also, a line starting with space followed by a '#' cannot\nhave any meaning other than being a comment, hence it doesn't harm to\naccept them as comments.\n\nLargely based on patch by: Junio C Hamano \n---\n\nI've re-added the last paragraph about whether it's good to allow\nindented comments, so that someone running \"git blame\" on my test get\nmore explanation about why this is deliberate.\n\nI'm sending this with me in the author field, but feel free to take\nback the ownership, you have about as much code as I do in this patch.\n\n git-rebase--interactive.sh | 62 ++++++++++++++++++++-----------------------\n t/t3404-rebase-interactive.sh | 15 +++++++++++\n 2 files changed, 44 insertions(+), 33 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f01637b..4ebd547 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -857,7 +857,8 @@ add_exec_commands () {\n # Check if the SHA-1 passed as an argument is a\n # correct one, if not then print $2 in \"$todo\".badsha\n # $1: the SHA-1 to test\n-# $2: the line to display if incorrect SHA-1\n+# $2: the line number of the input\n+# $3: the input filename\n check_commit_sha () {\n \tbadsha=0\n \tif test -z $1\n@@ -873,9 +874,10 @@ check_commit_sha () {\n \n \tif test $badsha -ne 0\n \tthen\n+\t\tline=\"$(sed -n -e \"${2}p\" \"$3\")\"\n \t\twarn \"Warning: the SHA-1 is missing or isn't\" \\\n \t\t\t\"a commit in the following line:\"\n-\t\twarn \" - $2\"\n+\t\twarn \" - $line\"\n \t\twarn\n \tfi\n \n@@ -886,37 +888,31 @@ check_commit_sha () {\n # from the todolist in stdin\n check_bad_cmd_and_sha () {\n \tretval=0\n-\tgit stripspace --strip-comments |\n-\t(\n-\t\twhile read -r line\n-\t\tdo\n-\t\t\tIFS=' '\n-\t\t\tset -- $line\n-\t\t\tcommand=$1\n-\t\t\tsha1=$2\n-\n-\t\t\tcase $command in\n-\t\t\t''|noop|x|\"exec\")\n-\t\t\t\t# Doesn't expect a SHA-1\n-\t\t\t\t;;\n-\t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n-\t\t\t\tif ! check_commit_sha $sha1 \"$line\"\n-\t\t\t\tthen\n-\t\t\t\t\tretval=1\n-\t\t\t\tfi\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\twarn \"Warning: the command isn't recognized\" \\\n-\t\t\t\t\t\"in the following line:\"\n-\t\t\t\twarn \" - $line\"\n-\t\t\t\twarn\n+\tlineno=0\n+\twhile read -r command sha1 rest\n+\tdo\n+\t\tlineno=$(( $lineno + 1 ))\n+\t\tcase $command in\n+\t\t\"$comment_char\"*|''|noop|x|exec)\n+\t\t\t# Doesn't expect a SHA-1\n+\t\t\t;;\n+\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\tif ! check_commit_sha \"$sha1\" \"$lineno\" \"$1\"\n+\t\t\tthen\n \t\t\t\tretval=1\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\tdone\n-\n-\t\treturn $retval\n-\t)\n+\t\t\tfi\n+\t\t\t;;\n+\t\t*)\n+\t\t\tline=\"$(sed -n -e \"${lineno}p\" \"$1\")\"\n+\t\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\t\"in the following line:\"\n+\t\t\twarn \" - $line\"\n+\t\t\twarn\n+\t\t\tretval=1\n+\t\t\t;;\n+\t\tesac\n+\tdone <\"$1\"\n+\treturn $retval\n }\n \n # Print the list of the SHA-1 of the commits\n@@ -1010,7 +1006,7 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n-\tif ! check_bad_cmd_and_sha <\"$todo\"\n+\tif ! check_bad_cmd_and_sha \"$todo\"\n \tthen\n \t\traise_error=t\n \tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d26e3f5..3da3ba3 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'tabs and spaces are accepted in the todolist' '\n+\trebase_setup_and_clean indented-comment &&\n+\twrite_script add-indent.sh <<-\\EOF &&\n+\t(\n+\t\t# Turn single spaces into space/tab mix\n+\t\tsed \"1s/ /\\t/g; 2s/ / /g; 3s/ / \\t/g\" \"$1\"\n+\t\tprintf \"\\n\\t# comment\\n #more\\n\\t # comment\\n\"\n+\t) >$1.new\n+\tmv \"$1.new\" \"$1\"\n+\tEOF\n+\ttest_set_editor \"$(pwd)/add-indent.sh\" &&\n+\tgit rebase -i HEAD^^^ &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n cat >expect < wrote:\n> 804098bb (git rebase -i: add static check for commands and SHA-1,\n> 2015-06-29) tried to check all insns before running any in the todo\n> list, but it did so by implementing its own parser that is a lot\n> stricter than necessary. We used to allow lines that are indented\n> (including comment lines), and we used to allow a whitespace between\n> the insn and the commit object name to be HT, among other things,\n> that are flagged as an invalid line by mistake.\n>\n> Fix this by using the same tokenizer that is used to parse the todo\n> list file in the new check.\n>\n> Whether it's a good thing to accept indented comments is\n> debatable (other commands like \"git commit\" do not accept them), but we\n> already accepted them in the past, and some people and scripts rely on\n> this behavior. Also, a line starting with space followed by a '#' cannot\n> have any meaning other than being a comment, hence it doesn't harm to\n> accept them as comments.\n>\n> Largely based on patch by: Junio C Hamano \n\nMissing sign-off.\n"},{"id":"270955","messageId":"1443643852-17578-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"40456","inReplyTo":"CAPig+cTadBNcTRHzTti-WuQSz=hRUsJYK1w9xiW6m=uOYCdCMQ@mail.gmail.com","subject":"[PATCH v2] rebase-i: loosen over-eager check_bad_cmd check","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-09-30T20:10:52Z","receivedAt":"2015-09-30T20:10:52Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"804098bb (git rebase -i: add static check for commands and SHA-1,\n2015-06-29) tried to check all insns before running any in the todo\nlist, but it did so by implementing its own parser that is a lot\nstricter than necessary. We used to allow lines that are indented\n(including comment lines), and we used to allow a whitespace between\nthe insn and the commit object name to be HT, among other things,\nthat are flagged as an invalid line by mistake.\n\nFix this by using the same tokenizer that is used to parse the todo\nlist file in the new check.\n\nWhether it's a good thing to accept indented comments is\ndebatable (other commands like \"git commit\" do not accept them), but we\nalready accepted them in the past, and some people and scripts rely on\nthis behavior. Also, a line starting with space followed by a '#' cannot\nhave any meaning other than being a comment, hence it doesn't harm to\naccept them as comments.\n\nLargely based on patch by: Junio C Hamano \nSigned-off-by: Matthieu Moy \n---\nOops, missing signed-off-by added.\n\n git-rebase--interactive.sh | 62 ++++++++++++++++++++-----------------------\n t/t3404-rebase-interactive.sh | 15 +++++++++++\n 2 files changed, 44 insertions(+), 33 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f01637b..4ebd547 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -857,7 +857,8 @@ add_exec_commands () {\n # Check if the SHA-1 passed as an argument is a\n # correct one, if not then print $2 in \"$todo\".badsha\n # $1: the SHA-1 to test\n-# $2: the line to display if incorrect SHA-1\n+# $2: the line number of the input\n+# $3: the input filename\n check_commit_sha () {\n \tbadsha=0\n \tif test -z $1\n@@ -873,9 +874,10 @@ check_commit_sha () {\n \n \tif test $badsha -ne 0\n \tthen\n+\t\tline=\"$(sed -n -e \"${2}p\" \"$3\")\"\n \t\twarn \"Warning: the SHA-1 is missing or isn't\" \\\n \t\t\t\"a commit in the following line:\"\n-\t\twarn \" - $2\"\n+\t\twarn \" - $line\"\n \t\twarn\n \tfi\n \n@@ -886,37 +888,31 @@ check_commit_sha () {\n # from the todolist in stdin\n check_bad_cmd_and_sha () {\n \tretval=0\n-\tgit stripspace --strip-comments |\n-\t(\n-\t\twhile read -r line\n-\t\tdo\n-\t\t\tIFS=' '\n-\t\t\tset -- $line\n-\t\t\tcommand=$1\n-\t\t\tsha1=$2\n-\n-\t\t\tcase $command in\n-\t\t\t''|noop|x|\"exec\")\n-\t\t\t\t# Doesn't expect a SHA-1\n-\t\t\t\t;;\n-\t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n-\t\t\t\tif ! check_commit_sha $sha1 \"$line\"\n-\t\t\t\tthen\n-\t\t\t\t\tretval=1\n-\t\t\t\tfi\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\twarn \"Warning: the command isn't recognized\" \\\n-\t\t\t\t\t\"in the following line:\"\n-\t\t\t\twarn \" - $line\"\n-\t\t\t\twarn\n+\tlineno=0\n+\twhile read -r command sha1 rest\n+\tdo\n+\t\tlineno=$(( $lineno + 1 ))\n+\t\tcase $command in\n+\t\t\"$comment_char\"*|''|noop|x|exec)\n+\t\t\t# Doesn't expect a SHA-1\n+\t\t\t;;\n+\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\tif ! check_commit_sha \"$sha1\" \"$lineno\" \"$1\"\n+\t\t\tthen\n \t\t\t\tretval=1\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\tdone\n-\n-\t\treturn $retval\n-\t)\n+\t\t\tfi\n+\t\t\t;;\n+\t\t*)\n+\t\t\tline=\"$(sed -n -e \"${lineno}p\" \"$1\")\"\n+\t\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\t\"in the following line:\"\n+\t\t\twarn \" - $line\"\n+\t\t\twarn\n+\t\t\tretval=1\n+\t\t\t;;\n+\t\tesac\n+\tdone <\"$1\"\n+\treturn $retval\n }\n \n # Print the list of the SHA-1 of the commits\n@@ -1010,7 +1006,7 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n-\tif ! check_bad_cmd_and_sha <\"$todo\"\n+\tif ! check_bad_cmd_and_sha \"$todo\"\n \tthen\n \t\traise_error=t\n \tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d26e3f5..3da3ba3 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'tabs and spaces are accepted in the todolist' '\n+\trebase_setup_and_clean indented-comment &&\n+\twrite_script add-indent.sh <<-\\EOF &&\n+\t(\n+\t\t# Turn single spaces into space/tab mix\n+\t\tsed \"1s/ /\\t/g; 2s/ / /g; 3s/ / \\t/g\" \"$1\"\n+\t\tprintf \"\\n\\t# comment\\n #more\\n\\t # comment\\n\"\n+\t) >$1.new\n+\tmv \"$1.new\" \"$1\"\n+\tEOF\n+\ttest_set_editor \"$(pwd)/add-indent.sh\" &&\n+\tgit rebase -i HEAD^^^ &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n cat >expect < writes:\n\n> Junio C Hamano writes:\n>\n>> +\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n>> +\t\t\tif ! check_commit_sha \"${rest%% *}\" \"$lineno\" \"$1\"\n>\n> This does not pass my \"tabs\" test, as it parses the sha1 out of the line\n> assuming it's separated with a space.\n\nYes, it was very much deliberate to match what transform_todo_ids does.\n\nI do not think we mind loosening this SP to SP/HT at all, but if we\nare going to go in that direction, I'd very much more prefer to do\n\n\t\"${rest%%[ ]*}\"\n\nhere _and_ in transform_todo_ids so that we can keep the identical\nparsing, which is the primary point of the \"regression fix\", rather\nthan doing this:\n\n> I changed it to\n>\n> \twhile read -r command sha1 rest\n>\n> which is a bit more lazy.\n\nwhich is to invite the two parsers drift apart over time.\n\nThanks.\n"},{"id":"270976","messageId":"1443687522-14311-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"40456","inReplyTo":"xmqq8u7n7ikl.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3 1/2] rebase-i: explicitly accept tab as separator in commands","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-10-01T08:18:41Z","receivedAt":"2015-10-01T08:18:41Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"The git-rebase-todo is parsed several times with different parsers. In\nprinciple, the user input is normalized by transform_todo_ids and\nfurther parsing can be stricter.\n\nIn case the user wrote\n\npick deadbeefcommit message\n\nthe parser of transform_todo_ids was considering the sha1 to be\n\"deadbeefcommit\", and was leaving the tab in the transformed sheet.\nIn practice, this went unnoticed since the actual command interpretation\nwas done later in do_next which did accept the tab as a separator.\n\nMake it explicit in the code of transform_todo_ids that tabs are\naccepted. This way, code that mimicks it will also accept tabs as\nseparator.\n\nA similar construct appears in skip_unnecessary_picks, but this one\ncomes after transform_todo_ids, hence reads the normalized format, so it\nneeds not be changed.\n\nSigned-off-by: Matthieu Moy \n---\nThis is new in v3.\n\n git-rebase--interactive.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f01637b..0d77429 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -729,8 +729,8 @@ transform_todo_ids () {\n \t\t\t# that do not have a SHA-1 at the beginning of $rest.\n \t\t\t;;\n \t\t*)\n-\t\t\tsha1=$(git rev-parse --verify --quiet \"$@\" ${rest%% *}) &&\n-\t\t\trest=\"$sha1 ${rest#* }\"\n+\t\t\tsha1=$(git rev-parse --verify --quiet \"$@\" ${rest%%[\t ]*}) &&\n+\t\t\trest=\"$sha1 ${rest#*[\t ]}\"\n \t\t\t;;\n \t\tesac\n \t\tprintf '%s\\n' \"$command${rest:+ }$rest\"\n-- \n2.5.0.402.g8854c44\n"},{"id":"270977","messageId":"1443687522-14311-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"40456","inReplyTo":"1443687522-14311-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH v3 2/2] rebase-i: loosen over-eager check_bad_cmd check","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-10-01T08:18:42Z","receivedAt":"2015-10-01T08:18:42Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"804098bb (git rebase -i: add static check for commands and SHA-1,\n2015-06-29) tried to check all insns before running any in the todo\nlist, but it did so by implementing its own parser that is a lot\nstricter than necessary. We used to allow lines that are indented\n(including comment lines), and we used to allow a whitespace between\nthe insn and the commit object name to be HT, among other things,\nthat are flagged as an invalid line by mistake.\n\nFix this by using the same tokenizer that is used to parse the todo\nlist file in the new check.\n\nWhether it's a good thing to accept indented comments is\ndebatable (other commands like \"git commit\" do not accept them), but we\nalready accepted them in the past, and some people and scripts rely on\nthis behavior. Also, a line starting with space followed by a '#' cannot\nhave any meaning other than being a comment, hence it doesn't harm to\naccept them as comments.\n\nLargely based on patch by: Junio C Hamano \n\nSigned-off-by: Matthieu Moy \n---\nI went back to Junio's version, plus [ ] (space-tab) instead of just\nspace to split the line.\n\n git-rebase--interactive.sh | 62 ++++++++++++++++++++-----------------------\n t/t3404-rebase-interactive.sh | 15 +++++++++++\n 2 files changed, 44 insertions(+), 33 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 0d77429..d65c06e 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -857,7 +857,8 @@ add_exec_commands () {\n # Check if the SHA-1 passed as an argument is a\n # correct one, if not then print $2 in \"$todo\".badsha\n # $1: the SHA-1 to test\n-# $2: the line to display if incorrect SHA-1\n+# $2: the line number of the input\n+# $3: the input filename\n check_commit_sha () {\n \tbadsha=0\n \tif test -z $1\n@@ -873,9 +874,10 @@ check_commit_sha () {\n \n \tif test $badsha -ne 0\n \tthen\n+\t\tline=\"$(sed -n -e \"${2}p\" \"$3\")\"\n \t\twarn \"Warning: the SHA-1 is missing or isn't\" \\\n \t\t\t\"a commit in the following line:\"\n-\t\twarn \" - $2\"\n+\t\twarn \" - $line\"\n \t\twarn\n \tfi\n \n@@ -886,37 +888,31 @@ check_commit_sha () {\n # from the todolist in stdin\n check_bad_cmd_and_sha () {\n \tretval=0\n-\tgit stripspace --strip-comments |\n-\t(\n-\t\twhile read -r line\n-\t\tdo\n-\t\t\tIFS=' '\n-\t\t\tset -- $line\n-\t\t\tcommand=$1\n-\t\t\tsha1=$2\n-\n-\t\t\tcase $command in\n-\t\t\t''|noop|x|\"exec\")\n-\t\t\t\t# Doesn't expect a SHA-1\n-\t\t\t\t;;\n-\t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n-\t\t\t\tif ! check_commit_sha $sha1 \"$line\"\n-\t\t\t\tthen\n-\t\t\t\t\tretval=1\n-\t\t\t\tfi\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\twarn \"Warning: the command isn't recognized\" \\\n-\t\t\t\t\t\"in the following line:\"\n-\t\t\t\twarn \" - $line\"\n-\t\t\t\twarn\n+\tlineno=0\n+\twhile read -r command rest\n+\tdo\n+\t\tlineno=$(( $lineno + 1 ))\n+\t\tcase $command in\n+\t\t\"$comment_char\"*|''|noop|x|exec)\n+\t\t\t# Doesn't expect a SHA-1\n+\t\t\t;;\n+\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\tif ! check_commit_sha \"${rest%%[ \t]*}\" \"$lineno\" \"$1\"\n+\t\t\tthen\n \t\t\t\tretval=1\n-\t\t\t\t;;\n-\t\t\tesac\n-\t\tdone\n-\n-\t\treturn $retval\n-\t)\n+\t\t\tfi\n+\t\t\t;;\n+\t\t*)\n+\t\t\tline=\"$(sed -n -e \"${lineno}p\" \"$1\")\"\n+\t\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\t\"in the following line:\"\n+\t\t\twarn \" - $line\"\n+\t\t\twarn\n+\t\t\tretval=1\n+\t\t\t;;\n+\t\tesac\n+\tdone <\"$1\"\n+\treturn $retval\n }\n \n # Print the list of the SHA-1 of the commits\n@@ -1010,7 +1006,7 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n-\tif ! check_bad_cmd_and_sha <\"$todo\"\n+\tif ! check_bad_cmd_and_sha \"$todo\"\n \tthen\n \t\traise_error=t\n \tfi\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d26e3f5..3da3ba3 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '\n \ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'tabs and spaces are accepted in the todolist' '\n+\trebase_setup_and_clean indented-comment &&\n+\twrite_script add-indent.sh <<-\\EOF &&\n+\t(\n+\t\t# Turn single spaces into space/tab mix\n+\t\tsed \"1s/ /\\t/g; 2s/ / /g; 3s/ / \\t/g\" \"$1\"\n+\t\tprintf \"\\n\\t# comment\\n #more\\n\\t # comment\\n\"\n+\t) >$1.new\n+\tmv \"$1.new\" \"$1\"\n+\tEOF\n+\ttest_set_editor \"$(pwd)/add-indent.sh\" &&\n+\tgit rebase -i HEAD^^^ &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n cat >expect <