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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Matthieu Moy <Matthieu.Moy@xxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]