[PATCH] rebase: accept indented comments (fixes regression)

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] rebase: accept indented comments (fixes regression)

Matthieu Moy
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 <[hidden email]>
---
Junio C Hamano <[hidden email]> writes:

> Junio C Hamano <[hidden email]> 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.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase: accept indented comments (fixes regression)

Remi Galan Alfonso
Matthieu Moy <[hidden email]> writes:

> 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 <[hidden email]>

Thank you for the patch, and sorry for the introduced regression.

RĂ©mi

> ---
> Junio C Hamano <[hidden email]> writes:
>
> > Junio C Hamano <[hidden email]> 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.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase: accept indented comments (fixes regression)

Junio C Hamano
In reply to this post by Matthieu Moy
Matthieu Moy <[hidden email]> writes:

> 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.

I cannot help thinking that this is sidestepping the real issue.

The real issue, I think, is that the new check tokenises the input
differently from how the expand_todo_ids -> transform_todo_ids
callchain parses it.  The problem Nazri Ramliy noticed about the new
check that does not ignore the indentation is merely one aspect of
it.

Stripping leading whilespaces with sed may ignore indented anything
and help Nazri's script, but 804098b tightened checks to disallow
other things that we historically allowed, e.g. if you replace SP
between "pick" and the commit object name with an HT, the new check
will not notice that HT is also a perfectly good token separator
character and barfs.

I am actually tempted to say that we should revert 804098b, which is
the simplest fix.

If we want "check everything before doing a single thing" mode, the
right way to do it would be to base the check on the same loop as
transform_todo_ids (one way to do so would be to give a third mode
to that helper function, but I do not think we mind a small code
duplication).

As far as I can tell, the hand-rolled parsing is there only in oder
to report the incoming $line as-is.  It is much easier to just
identify with which line number the location of the problem, and
show it when it is necessary from the original source, and we do not
care about performance in the error codepath.

Perhaps something along these lines instead, with your new tests
added in?

 git-rebase--interactive.sh | 62 ++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dcc3401..ae1806a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -849,7 +849,8 @@ add_exec_commands () {
 # Check if the SHA-1 passed as an argument is a
 # correct one, if not then print $2 in "$todo".badsha
 # $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
 check_commit_sha () {
  badsha=0
  if test -z $1
@@ -865,9 +866,10 @@ check_commit_sha () {
 
  if test $badsha -ne 0
  then
+ line="$(sed -n -e "${2}p" "$3")"
  warn "Warning: the SHA-1 is missing or isn't" \
  "a commit in the following line:"
- warn " - $2"
+ warn " - $line"
  warn
  fi
 
@@ -878,37 +880,31 @@ check_commit_sha () {
 # from the todolist in stdin
 check_bad_cmd_and_sha () {
  retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "${rest%% *}" "$lineno" "$1"
+ then
  retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
 }
 
 # Print the list of the SHA-1 of the commits
@@ -1002,7 +998,7 @@ check_todo_list () {
  ;;
  esac
 
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
  then
  raise_error=t
  fi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase: accept indented comments (fixes regression)

Matthieu Moy-2
Junio C Hamano <[hidden email]> writes:

> Matthieu Moy <[hidden email]> writes:
>
>> 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.
>
> I cannot help thinking that this is sidestepping the real issue.
>
> The real issue, I think, is that the new check tokenises the input
> differently from how the expand_todo_ids -> transform_todo_ids
> callchain parses it.  The problem Nazri Ramliy noticed about the new
> check that does not ignore the indentation is merely one aspect of
> it.

Right.

> Stripping leading whilespaces with sed may ignore indented anything
> and help Nazri's script, but 804098b tightened checks to disallow
> other things that we historically allowed, e.g. if you replace SP
> between "pick" and the commit object name with an HT, the new check
> will not notice that HT is also a perfectly good token separator
> character and barfs.

Indeed. I'm adding a test for that too.

> I am actually tempted to say that we should revert 804098b, which is
> the simplest fix.

I think the commit has value, and reverting it makes the "drop" command
essentially useless.

> As far as I can tell, the hand-rolled parsing is there only in oder
> to report the incoming $line as-is.

Indeed, I remember finding the parsing code weird when I reviewed it,
and the reason was to provide the exact line.

> It is much easier to just identify with which line number the location
> of the problem, and show it when it is necessary from the original
> source, and we do not care about performance in the error codepath.

Agreed.

> Perhaps something along these lines instead, with your new tests
> added in?

Sounds good, yes. I'll send a patch with this and my updated tests.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase: accept indented comments (fixes regression)

Junio C Hamano
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> I am actually tempted to say that we should revert 804098b, which is
> the simplest fix.
>
> If we want "check everything before doing a single thing" mode, the
> right way to do it would be to base the check on the same loop as
> transform_todo_ids (one way to do so would be to give a third mode
> to that helper function, but I do not think we mind a small code
> duplication).
> ...

So here is a reroll, which is now minimally tested.

-- >8 --
Subject: [PATCH] rebase-i: loosen over-eager check_bad_cmd check

804098bb (git rebase -i: add static check for commands and SHA-1,
2015-06-29) tried to check all insns before running any in the todo
list, but it did so by implementing its own parser that is a lot
stricter than necessary.  We used to allow lines that are indented
(including comment lines), and we used to allow a whitespace between
the insn and the commit object name to be HT, among other things,
that are flagged as an invalid line by mistake.

Fix this by using the same tokenizer that is used to parse the todo
list file in the new check.  A new test is by Matthieu Moy.

Signed-off-by: Junio C Hamano <[hidden email]>
---
 git-rebase--interactive.sh    | 62 ++++++++++++++++++++-----------------------
 t/t3404-rebase-interactive.sh | 10 +++++++
 2 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dcc3401..ae1806a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -849,7 +849,8 @@ add_exec_commands () {
 # Check if the SHA-1 passed as an argument is a
 # correct one, if not then print $2 in "$todo".badsha
 # $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
 check_commit_sha () {
  badsha=0
  if test -z $1
@@ -865,9 +866,10 @@ check_commit_sha () {
 
  if test $badsha -ne 0
  then
+ line="$(sed -n -e "${2}p" "$3")"
  warn "Warning: the SHA-1 is missing or isn't" \
  "a commit in the following line:"
- warn " - $2"
+ warn " - $line"
  warn
  fi
 
@@ -878,37 +880,31 @@ check_commit_sha () {
 # from the todolist in stdin
 check_bad_cmd_and_sha () {
  retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "${rest%% *}" "$lineno" "$1"
+ then
  retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
 }
 
 # Print the list of the SHA-1 of the commits
@@ -1002,7 +998,7 @@ check_todo_list () {
  ;;
  esac
 
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
  then
  raise_error=t
  fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ebdab4b..2437a3c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1206,6 +1206,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-273-g484a0d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase: accept indented comments (fixes regression)

Junio C Hamano
In reply to this post by Matthieu Moy-2
Matthieu Moy <[hidden email]> writes:

> Sounds good, yes. I'll send a patch with this and my updated tests.

Thanks.  I think our mails crossed, so I'd discard my copy that
lacks the new test you wrote.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase: accept indented comments (fixes regression)

Matthieu Moy-2
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> writes:

> + pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> + if ! check_commit_sha "${rest%% *}" "$lineno" "$1"

This does not pass my "tabs" test, as it parses the sha1 out of the line
assuming it's separated with a space. It's used in other places of the
code, but tabs still seem to work more or less by chance (they are not
parsed properly by transform_todo_ids, but then they are understood by
do_next).

I changed it to

        while read -r command sha1 rest

which is a bit more lazy.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH] rebase-i: loosen over-eager check_bad_cmd check

Matthieu Moy
In reply to this post by Junio C Hamano
804098bb (git rebase -i: add static check for commands and SHA-1,
2015-06-29) tried to check all insns before running any in the todo
list, but it did so by implementing its own parser that is a lot
stricter than necessary.  We used to allow lines that are indented
(including comment lines), and we used to allow a whitespace between
the insn and the commit object name to be HT, among other things,
that are flagged as an invalid line by mistake.

Fix this by using the same tokenizer that is used to parse the todo
list file in the new check.

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.

Largely based on patch by: Junio C Hamano <[hidden email]>
---

I've re-added the last paragraph about whether it's good to allow
indented comments, so that someone running "git blame" on my test get
more explanation about why this is deliberate.

I'm sending this with me in the author field, but feel free to take
back the ownership, you have about as much code as I do in this patch.

 git-rebase--interactive.sh    | 62 ++++++++++++++++++++-----------------------
 t/t3404-rebase-interactive.sh | 15 +++++++++++
 2 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f01637b..4ebd547 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -857,7 +857,8 @@ add_exec_commands () {
 # Check if the SHA-1 passed as an argument is a
 # correct one, if not then print $2 in "$todo".badsha
 # $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
 check_commit_sha () {
  badsha=0
  if test -z $1
@@ -873,9 +874,10 @@ check_commit_sha () {
 
  if test $badsha -ne 0
  then
+ line="$(sed -n -e "${2}p" "$3")"
  warn "Warning: the SHA-1 is missing or isn't" \
  "a commit in the following line:"
- warn " - $2"
+ warn " - $line"
  warn
  fi
 
@@ -886,37 +888,31 @@ check_commit_sha () {
 # from the todolist in stdin
 check_bad_cmd_and_sha () {
  retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command sha1 rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "$sha1" "$lineno" "$1"
+ then
  retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
 }
 
 # Print the list of the SHA-1 of the commits
@@ -1010,7 +1006,7 @@ check_todo_list () {
  ;;
  esac
 
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
  then
  raise_error=t
  fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d26e3f5..3da3ba3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '
  test C = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'tabs and spaces are accepted in the todolist' '
+ rebase_setup_and_clean indented-comment &&
+ write_script add-indent.sh <<-\EOF &&
+ (
+ # Turn single spaces into space/tab mix
+ sed "1s/ /\t/g; 2s/ /  /g; 3s/ / \t/g" "$1"
+ printf "\n\t# comment\n #more\n\t # comment\n"
+ ) >$1.new
+ mv "$1.new" "$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.gb06d8e9.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase-i: loosen over-eager check_bad_cmd check

Eric Sunshine
On Wed, Sep 30, 2015 at 4:01 PM, Matthieu Moy <[hidden email]> wrote:

> 804098bb (git rebase -i: add static check for commands and SHA-1,
> 2015-06-29) tried to check all insns before running any in the todo
> list, but it did so by implementing its own parser that is a lot
> stricter than necessary.  We used to allow lines that are indented
> (including comment lines), and we used to allow a whitespace between
> the insn and the commit object name to be HT, among other things,
> that are flagged as an invalid line by mistake.
>
> Fix this by using the same tokenizer that is used to parse the todo
> list file in the new check.
>
> 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.
>
> Largely based on patch by: Junio C Hamano <[hidden email]>

Missing sign-off.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] rebase-i: loosen over-eager check_bad_cmd check

Matthieu Moy
804098bb (git rebase -i: add static check for commands and SHA-1,
2015-06-29) tried to check all insns before running any in the todo
list, but it did so by implementing its own parser that is a lot
stricter than necessary.  We used to allow lines that are indented
(including comment lines), and we used to allow a whitespace between
the insn and the commit object name to be HT, among other things,
that are flagged as an invalid line by mistake.

Fix this by using the same tokenizer that is used to parse the todo
list file in the new check.

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.

Largely based on patch by: Junio C Hamano <[hidden email]>
Signed-off-by: Matthieu Moy <[hidden email]>
---
Oops, missing signed-off-by added.

 git-rebase--interactive.sh    | 62 ++++++++++++++++++++-----------------------
 t/t3404-rebase-interactive.sh | 15 +++++++++++
 2 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f01637b..4ebd547 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -857,7 +857,8 @@ add_exec_commands () {
 # Check if the SHA-1 passed as an argument is a
 # correct one, if not then print $2 in "$todo".badsha
 # $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
 check_commit_sha () {
  badsha=0
  if test -z $1
@@ -873,9 +874,10 @@ check_commit_sha () {
 
  if test $badsha -ne 0
  then
+ line="$(sed -n -e "${2}p" "$3")"
  warn "Warning: the SHA-1 is missing or isn't" \
  "a commit in the following line:"
- warn " - $2"
+ warn " - $line"
  warn
  fi
 
@@ -886,37 +888,31 @@ check_commit_sha () {
 # from the todolist in stdin
 check_bad_cmd_and_sha () {
  retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command sha1 rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "$sha1" "$lineno" "$1"
+ then
  retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
 }
 
 # Print the list of the SHA-1 of the commits
@@ -1010,7 +1006,7 @@ check_todo_list () {
  ;;
  esac
 
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
  then
  raise_error=t
  fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d26e3f5..3da3ba3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '
  test C = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'tabs and spaces are accepted in the todolist' '
+ rebase_setup_and_clean indented-comment &&
+ write_script add-indent.sh <<-\EOF &&
+ (
+ # Turn single spaces into space/tab mix
+ sed "1s/ /\t/g; 2s/ /  /g; 3s/ / \t/g" "$1"
+ printf "\n\t# comment\n #more\n\t # comment\n"
+ ) >$1.new
+ mv "$1.new" "$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.5.0.402.g8854c44

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] rebase: accept indented comments (fixes regression)

Junio C Hamano
In reply to this post by Matthieu Moy-2
Matthieu Moy <[hidden email]> writes:

> Junio C Hamano <[hidden email]> writes:
>
>> + pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
>> + if ! check_commit_sha "${rest%% *}" "$lineno" "$1"
>
> This does not pass my "tabs" test, as it parses the sha1 out of the line
> assuming it's separated with a space.

Yes, it was very much deliberate to match what transform_todo_ids does.

I do not think we mind loosening this SP to SP/HT at all, but if we
are going to go in that direction, I'd very much more prefer to do

        "${rest%%[     ]*}"

here _and_ in transform_todo_ids so that we can keep the identical
parsing, which is the primary point of the "regression fix", rather
than doing this:

> I changed it to
>
> while read -r command sha1 rest
>
> which is a bit more lazy.

which is to invite the two parsers drift apart over time.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 1/2] rebase-i: explicitly accept tab as separator in commands

Matthieu Moy
The git-rebase-todo is parsed several times with different parsers. In
principle, the user input is normalized by transform_todo_ids and
further parsing can be stricter.

In case the user wrote

pick deadbeef<TAB>commit message

the parser of transform_todo_ids was considering the sha1 to be
"deadbeef<TAB>commit", and was leaving the tab in the transformed sheet.
In practice, this went unnoticed since the actual command interpretation
was done later in do_next which did accept the tab as a separator.

Make it explicit in the code of transform_todo_ids that tabs are
accepted. This way, code that mimicks it will also accept tabs as
separator.

A similar construct appears in skip_unnecessary_picks, but this one
comes after transform_todo_ids, hence reads the normalized format, so it
needs not be changed.

Signed-off-by: Matthieu Moy <[hidden email]>
---
This is new in v3.

 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 f01637b..0d77429 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -729,8 +729,8 @@ transform_todo_ids () {
  # that do not have a SHA-1 at the beginning of $rest.
  ;;
  *)
- sha1=$(git rev-parse --verify --quiet "$@" ${rest%% *}) &&
- rest="$sha1 ${rest#* }"
+ sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[ ]*}) &&
+ rest="$sha1 ${rest#*[ ]}"
  ;;
  esac
  printf '%s\n' "$command${rest:+ }$rest"
--
2.5.0.402.g8854c44

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 2/2] rebase-i: loosen over-eager check_bad_cmd check

Matthieu Moy
804098bb (git rebase -i: add static check for commands and SHA-1,
2015-06-29) tried to check all insns before running any in the todo
list, but it did so by implementing its own parser that is a lot
stricter than necessary.  We used to allow lines that are indented
(including comment lines), and we used to allow a whitespace between
the insn and the commit object name to be HT, among other things,
that are flagged as an invalid line by mistake.

Fix this by using the same tokenizer that is used to parse the todo
list file in the new check.

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.

Largely based on patch by: Junio C Hamano <[hidden email]>

Signed-off-by: Matthieu Moy <[hidden email]>
---
I went back to Junio's version, plus [ ] (space-tab) instead of just
space to split the line.

 git-rebase--interactive.sh    | 62 ++++++++++++++++++++-----------------------
 t/t3404-rebase-interactive.sh | 15 +++++++++++
 2 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0d77429..d65c06e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -857,7 +857,8 @@ add_exec_commands () {
 # Check if the SHA-1 passed as an argument is a
 # correct one, if not then print $2 in "$todo".badsha
 # $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
 check_commit_sha () {
  badsha=0
  if test -z $1
@@ -873,9 +874,10 @@ check_commit_sha () {
 
  if test $badsha -ne 0
  then
+ line="$(sed -n -e "${2}p" "$3")"
  warn "Warning: the SHA-1 is missing or isn't" \
  "a commit in the following line:"
- warn " - $2"
+ warn " - $line"
  warn
  fi
 
@@ -886,37 +888,31 @@ check_commit_sha () {
 # from the todolist in stdin
 check_bad_cmd_and_sha () {
  retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "${rest%%[ ]*}" "$lineno" "$1"
+ then
  retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
 }
 
 # Print the list of the SHA-1 of the commits
@@ -1010,7 +1006,7 @@ check_todo_list () {
  ;;
  esac
 
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
  then
  raise_error=t
  fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d26e3f5..3da3ba3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '
  test C = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+test_expect_success 'tabs and spaces are accepted in the todolist' '
+ rebase_setup_and_clean indented-comment &&
+ write_script add-indent.sh <<-\EOF &&
+ (
+ # Turn single spaces into space/tab mix
+ sed "1s/ /\t/g; 2s/ /  /g; 3s/ / \t/g" "$1"
+ printf "\n\t# comment\n #more\n\t # comment\n"
+ ) >$1.new
+ mv "$1.new" "$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.5.0.402.g8854c44

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html