From: Johannes Sixt <j.sixt@viscovery.net>
Date: Thu, 11 Oct 2007 06:19:06 GMT
Subject: Re: [PATCH] Fixing path quoting issues
Message-ID: <470DC05A.8020209@viscovery.net>
In-Reply-To: <11920508172434-git-send-email-jon.delStrother@bestbefore.tv>

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