{"thread":{"id":"40451","subject":"2.6.0: Comment in rebase instruction has become too rigid","startedAt":"2015-09-29T14:02:37Z","lastAt":"2015-09-29T18:31:40Z","messageCount":6,"participants":["Nazri Ramliy","Matthieu Moy","Junio C Hamano","Ralf Thielow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"270911","messageId":"CAEY4ZpN0C96jU4Tgvqt-hWEviR-sapDoyAv88H+opPSr-cs=mg@mail.gmail.com","threadId":"40451","inReplyTo":null,"subject":"2.6.0: Comment in rebase instruction has become too rigid","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2015-09-29T14:02:37Z","receivedAt":"2015-09-29T14:02:37Z","isPatch":false,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"Hi,\n\nI noticed that the format of the comment lines in a rebase instruction\nsheet has become stricter - it could no longer begin with spaces or\ntabs. The comment char (\"#\" for example) has to appear on the first\ncolumn.\n\nThis break my little script (activated via some key binding in my\n$EDITOR) for adding the list of modified files under each \"pick\"\ncommand. The way I have it setup is something like this, given the\nfollowing rebase intruction:\n\n pick deadbeef some commit message\n pick cafebabe another commit message\n\nI'd hit that key in my editor that filters the pick instructions add\ninserts the list of the modified files in each commit so that the\ninstruction sheet becomes like this:\n\n pick deadbeef some commit message\n # M path/to/foo.txt | 15 ++++----------\n pick cafebabe another commit message\n # M bar.txt | 2 +-\n\n\nIIRC before git 2.6.0 this worked fine. With git 2.6.0 the rebase\nstops midway with warning about invalid instruction due to the now\nno-longer recognized indented comments.\n\nI could work around this by changing my script so that it removes the\nindentation prefix so that the instruction would become like this:\n\n pick deadbeef some commit message\n # M path/to/foo.txt | 15 ++++----------\n pick cafebabe another commit message\n # M bar.txt | 2 +-\n\nbut this would make it harder to read because of the increased clutter\nbetween the rebase instructions and the informative \"what files were\nchanged in this commit\" comment.\n\nLooking at git-rebase--interactive.sh it seems that this is due to\n\"git stripspace --strip-comments\".\n\nWould it be okay if the behavior is reverted to the old one - which is\nto recognize indented comments in the rebase instruction?\n\nNazri\n"},{"id":"270913","messageId":"vpqr3lhb719.fsf@grenoble-inp.fr","threadId":"40451","inReplyTo":"CAEY4ZpN0C96jU4Tgvqt-hWEviR-sapDoyAv88H+opPSr-cs=mg@mail.gmail.com","subject":"Re: 2.6.0: Comment in rebase instruction has become too rigid","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-29T15:05:06Z","receivedAt":"2015-09-29T15:05:06Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Nazri Ramliy writes:\n\n> I'd hit that key in my editor that filters the pick instructions add\n> inserts the list of the modified files in each commit so that the\n> instruction sheet becomes like this:\n>\n> pick deadbeef some commit message\n> # M path/to/foo.txt | 15 ++++----------\n> pick cafebabe another commit message\n> # M bar.txt | 2 +-\n>\n>\n> IIRC before git 2.6.0 this worked fine.\n\nConfirmed: Git 2.1.4 accepts this, 2.6 doesn't:\n\nWarning: the command isn't recognized in the following line:\n - # pick dbafac11052a0075233bdcf0b71f54d1503aa82d test\n\nYou can fix this with 'git rebase --edit-todo'.\nOr you can abort the rebase with 'git rebase --abort'.\n\nI didn't bisect, but I guess this was introduced in the series\nintroducing this check on the todolist before starting the bisection.\n\nActually, I think we accepted indented comments by mistake: the\nsemantics of comments in Git is usually that it must start at the first\ncolumn (try an indented # in a commit buffer, it's not a comment). But\nsince Git accepted it in the past, we should continue accepting it to\navoid breaking the user experience.\n\nNo time to send a patch right now, but I will hopefully be able to do\nthis within the next few days. It should be essentially a s/^ *// before\ncalling stripspaces.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"270916","messageId":"vpqzj0588i2.fsf@grenoble-inp.fr","threadId":"40451","inReplyTo":"vpqr3lhb719.fsf@grenoble-inp.fr","subject":"Re: 2.6.0: Comment in rebase instruction has become too rigid","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-29T17:01:41Z","receivedAt":"2015-09-29T17:01:41Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Matthieu Moy writes:\n\n> Nazri Ramliy writes:\n>\n>> I'd hit that key in my editor that filters the pick instructions add\n>> inserts the list of the modified files in each commit so that the\n>> instruction sheet becomes like this:\n>>\n>> pick deadbeef some commit message\n>> # M path/to/foo.txt | 15 ++++----------\n>> pick cafebabe another commit message\n>> # M bar.txt | 2 +-\n>>\n>>\n>> IIRC before git 2.6.0 this worked fine.\n>\n> Confirmed: Git 2.1.4 accepts this, 2.6 doesn't:\n>\n> Warning: the command isn't recognized in the following line:\n> - # pick dbafac11052a0075233bdcf0b71f54d1503aa82d test\n>\n> You can fix this with 'git rebase --edit-todo'.\n> Or you can abort the rebase with 'git rebase --abort'.\n>\n> I didn't bisect, but I guess this was introduced in the series\n> introducing this check on the todolist before starting the bisection.\n\nIndeed:\n\n804098bb30a5339cccb0be981a3e876245aa0ae5 is the first bad commit\ncommit 804098bb30a5339cccb0be981a3e876245aa0ae5\nAuthor: Galan Rémi \nDate: Mon Jun 29 22:20:32 2015 +0200\n\n git rebase -i: add static check for commands and SHA-1\n \n Check before the start of the rebasing if the commands exists, and for\n the commands expecting a SHA-1, check if the SHA-1 is present and\n corresponds to a commit. In case of error, print the error, stop git\n rebase and prompt the user to fix with 'git rebase --edit-todo' or to\n abort.\n \n This allows to avoid doing half of a rebase before finding an error\n and giving back what's left of the todo list to the user and prompt\n him to fix when it might be too late for him to do so (he might have\n to abort and restart the rebase).\n \n Signed-off-by: Galan Rémi \n Signed-off-by: Junio C Hamano \n\n:100644 100644 c26a200a6c0e7edd2b182b71af50df52179d295f dcc3401b5a8c45fd9c5ba474416eb4a6c3c9a29e M git-rebase--interactive.sh\n:040000 040000 3a2882c656f4a2ea3cfcba7e5afca79877c61295 522781ff8b31d55b76064d27f3d4326026721091 M t\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"270918","messageId":"xmqqk2r99jjp.fsf@gitster.mtv.corp.google.com","threadId":"40451","inReplyTo":"vpqzj0588i2.fsf@grenoble-inp.fr","subject":"Re: 2.6.0: Comment in rebase instruction has become too rigid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-29T18:17:46Z","receivedAt":"2015-09-29T18:17:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> Confirmed: Git 2.1.4 accepts this, 2.6 doesn't:\n>>\n>> Warning: the command isn't recognized in the following line:\n>> - # pick dbafac11052a0075233bdcf0b71f54d1503aa82d test\n>>\n>> You can fix this with 'git rebase --edit-todo'.\n>> Or you can abort the rebase with 'git rebase --abort'.\n>>\n>> I didn't bisect, but I guess this was introduced in the series\n>> introducing this check on the todolist before starting the bisection.\n>\n> Indeed:\n>\n> 804098bb30a5339cccb0be981a3e876245aa0ae5 is the first bad commit\n\nYup, before that series, expand_todo_ids -> transfom_todo_ids ended\nup reading each line with \"while read -r command rest\" loop and the\nwe did not honor the usual \"# at the beginning line is the comment\"\nconvention, which I think was a bug. With that commit, a separate\nstep in check_bad_cmd_and_sha1 uses a similar looking \"while read\"\nloop but forgets to take '#' into account.\n\nI know you alluded to preprocess what is fed to stripspace, but I\nwonder if we can remove the misguided call to stripspace in the\nfirst place and do something like the attached instead.\n\n git-rebase--interactive.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 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"},{"id":"270919","messageId":"xmqqeghh9iy2.fsf@gitster.mtv.corp.google.com","threadId":"40451","inReplyTo":"xmqqk2r99jjp.fsf@gitster.mtv.corp.google.com","subject":"Re: 2.6.0: Comment in rebase instruction has become too rigid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-29T18:30:45Z","receivedAt":"2015-09-29T18:30:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"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\nNah, that would not work, as I misread the \"split only at SP\" manual\nparsing of $line.\n\nI shouldn't be responding to the git list traffic on my vacation\nday, especially before my first caffeine X-<\n"},{"id":"270920","messageId":"CAN0XMOJDUMHX5WwVYoJVSuYKPHpf=+0Os=U34_zRDt1XPwPtQQ@mail.gmail.com","threadId":"40451","inReplyTo":"xmqqk2r99jjp.fsf@gitster.mtv.corp.google.com","subject":"Re: 2.6.0: Comment in rebase instruction has become too rigid","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2015-09-29T18:31:40Z","receivedAt":"2015-09-29T18:31:40Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"2015-09-29 20:17 GMT+02:00 Junio C Hamano :\n> Matthieu Moy writes:\n>\n>>> Confirmed: Git 2.1.4 accepts this, 2.6 doesn't:\n>>>\n>>> Warning: the command isn't recognized in the following line:\n>>> - # pick dbafac11052a0075233bdcf0b71f54d1503aa82d test\n>>>\n>>> You can fix this with 'git rebase --edit-todo'.\n>>> Or you can abort the rebase with 'git rebase --abort'.\n>>>\n>>> I didn't bisect, but I guess this was introduced in the series\n>>> introducing this check on the todolist before starting the bisection.\n>>\n>> Indeed:\n>>\n>> 804098bb30a5339cccb0be981a3e876245aa0ae5 is the first bad commit\n>\n> Yup, before that series, expand_todo_ids -> transfom_todo_ids ended\n> up reading each line with \"while read -r command rest\" loop and the\n> we did not honor the usual \"# at the beginning line is the comment\"\n> convention, which I think was a bug. With that commit, a separate\n> step in check_bad_cmd_and_sha1 uses a similar looking \"while read\"\n> loop but forgets to take '#' into account.\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\nIf so, I think we should use \"$comment_char\"* here.\n\n> # Doesn't expect a SHA-1\n> ;;\n> pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"}]}