Subject: Re: [PATCH] Fixing path quoting issues

From: Johannes Sixt <[email-masked]>
Date: Thu, 11 Oct 2007 08:19:06 +0200

Jonathan del Strother schrieb:
> +	cmt=`cat "$dotest/current"`

This is ok, but...

> +			prev_head="`cat \"$dotest/prev_head\"`"

... there are shells out there in the wild that will get badly confused by 
this sort of quoting and escaping. Butter use

	prev_head=$(cat "$dotest/prev_head")

> -VISUAL="$(pwd)/fake-editor.sh"
> +VISUAL="'$(pwd)/fake-editor.sh'"

Huh? This looks very wrong. What are the extra quotes needed for? If they 
are really needed, isn't this a bug in git-rebase--interactive.sh?

> -	 git-commit -F msg -m amending ."
> +	git-commit -F msg -m amending ."

You fix whitespace...

>  test_expect_success \
> -	"using message from other commit" \
> -	"git-commit -C HEAD^ ."
> +	 "using message from other commit" \
> +	 "git-commit -C HEAD^ ."

... and you break it. More of these follow. Don't do that, it makes patch 
review unnecessarily hard.

I question the usefulness of this patch. Why only fix breakage due to spaces 
in the path? What about single-quotes, double-quotes? IMHO, it's not too 
much of a burden for developers to require "sane" build directory paths.

-- Hannes