Re: [PATCH] rebase: accept indented comments (fixes regression)
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 15 quoted lines
> With Git <2.0.6, 'git rebase' used to accept lines starting with
> whitespaces followed with '#' as a comment. This was broken by
> 804098b (git rebase -i: add static check for commands and SHA-1,
> 2015-06-29), which introduced additional checks on the TODO-list using
> "git stripspaces" which only strips comments starting at the first
> column.
>
> Whether it's a good thing to accept indented comments is
> debatable (other commands like "git commit" do not accept them), but we
> already accepted them in the past, and some people and scripts rely on
> this behavior. Also, a line starting with space followed by a '#' cannot
> have any meaning other than being a comment, hence it doesn't harm to
> accept them as comments.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Thank you for the patch, and sorry for the introduced regression.
Rémi
Show 81 quoted lines
> ---
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> I know you alluded to preprocess what is fed to stripspace, but I
> >> wonder if we can remove the misguided call to stripspace in the
> >> first place and do something like the attached instead.
> >>
> >> git-rebase--interactive.sh | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> >> index f01637b..a64f77a 100644
> >> --- a/git-rebase--interactive.sh
> >> +++ b/git-rebase--interactive.sh
> >> @@ -886,7 +886,6 @@ check_commit_sha () {
> >> # from the todolist in stdin
> >> check_bad_cmd_and_sha () {
> >> retval=0
> >> - git stripspace --strip-comments |
> >> (
> >> while read -r line
> >> do
> >> @@ -896,7 +895,7 @@ check_bad_cmd_and_sha () {
> >> sha1=$2
> >>
> >> case $command in
> >> - ''|noop|x|"exec")
> >> + '#'*|''|noop|x|"exec")
> >> # Doesn't expect a SHA-1
> >> ;;
> >> pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> >
> > Nah, that would not work, as I misread the "split only at SP" manual
> > parsing of $line.
>
> OK, let's go for the solution I seem to be able to get right even with
> low cafeine ;-).
>
> git-rebase--interactive.sh | 3 +++
> t/t3404-rebase-interactive.sh | 10 ++++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f01637b..55adf78 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -886,6 +886,9 @@ check_commit_sha () {
> # from the todolist in stdin
> check_bad_cmd_and_sha () {
> retval=0
> + # git rebase -i accepts comments preceeded by spaces, while
> + # stripspace does not.
> + sed 's/^[[:space:]]*//' |
> git stripspace --strip-comments |
> (
> while read -r line
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d26e3f5..ac5bac3 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1227,6 +1227,16 @@ test_expect_success 'static check of bad command' '
> test C = $(git cat-file commit HEAD^ | sed -ne \$p)
> '
>
> +test_expect_success 'indented comments are accepted' '
> + rebase_setup_and_clean indented-comment &&
> + write_script add-indent.sh <<-\EOF &&
> + printf "\n \t # comment\n" >>$1
> + EOF
> + test_set_editor "$(pwd)/add-indent.sh" &&
> + git rebase -i HEAD^ &&
> + test E = $(git cat-file commit HEAD | sed -ne \$p)
> +'
> +
> cat >expect <<EOF
> Warning: the SHA-1 is missing or isn't a commit in the following line:
> - edit XXXXXXX False commit
> --
> 2.6.0.rc2.24.g231a9a1.dirtyRemi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>