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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Feb 7, 2014, 23:50 UTC
Message-ID: <20140207235058.GJ635004@vauxhall.crustytoothpaste.net>
In-Reply-To: <xmqqlhxrdgb5.fsf@gitster.dls.corp.google.com>

On Mon, Feb 03, 2014 at 01:42:06PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> > +	--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.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187