Re: [PATCH v3 8/9] rebase: add the --gpg-sign option

[Philip Oakley <philipoakley@iee.org>]

On 07/02/14 23:50, brian m. carlson wrote:
> On Mon, Feb 03, 2014 at 01:42:06PM -0800, Junio C Hamano wrote:
>>> +	--gpg-sign)
>>> +		gpg_sign_opt=-S
>>> +		;;
>>> +	--gpg-sign=*)
>>> +		# Try to quote only the argument, as this will appear in human-readable
>>> +		# output as well as being passed to commands.
>>> +		gpg_sign_opt="-S$(git rev-parse --sq-quote "${1#--gpg-sign=}" |
>>> +			sed 's/^ //')"
>>
>> Isn't an invocation of sed excessive?
>>
>> 	gpg_sign_opt=$(git rev-parse --sq-quote "${1#--gpg-sign=}") &&
>> 	gpg_sign_opt=-S${gpg_sign_opt# }
>>
>> if you really need to strip the leading SP, which I do not think is
>> a necessary thing to do.  It is sufficient to remove the SP before
>> the variable substitution in the human-readable messages, e.g.
>
> I'm not sure that command line parsing of "-S 'foo <x@example.tld>'"
> will work exactly as expected due to the fact that -S doesn't always
> take an argument.  Your suggestion to use # seems fine, though.
>
> I'm a little embarrassed to admit that in my fifteen years of Unix
> experience, I've never learned the variable modifiers for shell, so it
> didn't occur to me to use them in this case.  Guess it's time to learn
> them now.

Same here:

For other readers, I found most google results were only partial on the 
issue, missing the '#' symbol option. Here's a more complete ref 
http://www.gnu.org/software/bash/manual/bashref.html#Shell-Parameter-Expansion-1 
for fellow learners.

Philip
--

>