Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 10, 2015, 15:20 UTC
Message-ID: <vpq8ubrtws0.fsf@anie.imag.fr>
In-Reply-To: <1433931035-20011-3-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr>

Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:

Show 18 quoted lines

> +# from the todolist in stdin
> +check_bad_cmd_and_sha () {
> +	git stripspace --strip-comments |
> +	while read -r command sha1 rest
> +	do
> +		case $command in
> +		''|noop|x|"exec")
> +			;;
> +		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> +			if test -z $sha1
> +			then
> +				echo "$command $rest" >>"$todo".badsha
> +			else
> +				sha1_verif="$(git rev-parse --verify --quiet $sha1^{commit})"
> +				if test -z $sha1_verif
> +				then
> +					echo "$command $sha1 $rest" \
> +						>>"$todo".badsha

When you reach the right end of your screen because of indentation, cutting lines with \ is rarely the best option. Having 5 levels of indentation is a sign that you should make more functions.

How about:

check_bad_cmd_and_sha () {
	git stripspace --strip-comments |
	while read -r command sha1 rest
	do
		case $command in
		''|noop|x|"exec")
			;;
		pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
			check_commit_id $sha1
			;;
		*)
			record_bad_command $sha1
			;;
	esac
}

> +		*)
> +			if test -z $sha1
> +			then
> +				echo "$command" >>"$todo".badcmd

Avoid echo on user-supplied data.

Show 10 quoted lines

> +			else
> +				commit="$(git rev-list --oneline -1 --ignore-missing $sha1 2>/dev/null)"
> +				if test -z "$commit"
> +				then
> +					echo "$command $sha1 $rest" \
> +						>>"$todo".badcmd
> +				else
> +					echo "$command $commit" >>"$todo".badcmd
> +				fi
> +			fi

What are you trying to do here? It seems that you are trying to recover the line, but the line is your input, you shouldn't have to recompute it.

Why isn't printf '%s %s %s' "$command" "$sha1" "$rest" sufficient in all cases?

Maybe it would be better to read line by line (to avoid loosing whitespace information for example), like

	while read -r line
	do
		printf '%s' "$line" | read -r cmd sha1 rest
		case $sha1 in
			...

or maybe it's overkill.

Show 5 quoted lines

> +	check_bad_cmd_and_sha <"$todo"
> +
> +	if test -s "$todo".badsha
> +	then
> +		raiseError=t

We usually don't use camelCase in shell-scripts. raise_error would be the usual way to spell in in Git's codebase.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/