Re: [PATCH] rebase: accept indented comments (fixes regression)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2015, 18:05 UTC
- Message-ID
- <xmqqd1wz9402.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1443600661-19391-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 6 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.
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