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

2014-02-08 Thread Philip Oakley 

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





--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-02-07 Thread brian m. carlson 
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.

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


signature.asc
Description: Digital signature

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

2014-02-03 Thread Junio C Hamano 
brian m. carlson sand...@crustytoothpaste.net writes:

 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
 index 43c19e0..73d32dd 100644
 --- a/git-rebase--interactive.sh
 +++ b/git-rebase--interactive.sh
 @@ -181,7 +181,7 @@ exit_with_patch () {
   git rev-parse --verify HEAD  $amend
   warn You can amend the commit now, with
   warn
 - warn   git commit --amend
 + warn   git commit --amend $gpg_sign_opt
   warn
   warn Once you are satisfied with your changes, run
   warn
 @@ -248,7 +248,8 @@ pick_one () {
  
   test -d $rewritten 
   pick_one_preserving_merges $@  return
 - output eval git cherry-pick $strategy_args $empty_args $ff $@
 + output eval git cherry-pick ${gpg_sign_opt:+$gpg_sign_opt} \
 + $strategy_args $empty_args $ff $@

This uses $gpg_sign_opt on eval, which means that the variable's
contents must be properly shell quoted, e.g.

gpg_sign_opt='-S'\''brian m. carson sand...@c.net'\'

throughout this script, so that everything between the first
double-quote  and closing ket  is passed as a single parameter
without being broken up.

 @@ -359,7 +360,8 @@ pick_one_preserving_merges () {
   echo $sha1 $(git rev-parse HEAD^0)  
 $rewritten_list
   ;;
   *)
 - output eval git cherry-pick $strategy_args $@ ||
 + output eval git cherry-pick 
 ${gpg_sign_opt:+$gpg_sign_opt} \

And this part has the same expectation.  However...

 @@ -470,7 +472,8 @@ do_pick () {
  --no-post-rewrite -n -q -C $1 
   pick_one -n $1 
   git commit --allow-empty --allow-empty-message \
 ---amend --no-post-rewrite -n -q -C $1 ||
 +--amend --no-post-rewrite -n -q -C $1 \
 +${gpg_sign_opt:+$gpg_sign_opt} ||

This does not want that extra level of quoting.  It would want to
have something like this instead:

gpg_sign_opt='-Sbrian m. carson sand...@c.net'

I am not sure how you are managing these two conflicting needs of
the use sites.

 @@ -497,7 +500,7 @@ do_next () {
  
   mark_action_done
   do_pick $sha1 $rest
 - git commit --amend --no-post-rewrite || {
 + git commit --amend --no-post-rewrite 
 ${gpg_sign_opt:+$gpg_sign_opt} || {

Ditto.

 diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
 index e7d96de..5381857 100644
 --- a/git-rebase--merge.sh
 +++ b/git-rebase--merge.sh
 @@ -27,7 +27,7 @@ continue_merge () {
   cmt=`cat $state_dir/current`
   if ! git diff-index --quiet --ignore-submodules HEAD --
   then
 - if ! git commit --no-verify -C $cmt
 + if ! git commit ${gpg_sign_opt:+$gpg_sign_opt} --no-verify -C 
 $cmt

Ditto.

   then
   echo Commit failed, please do not call \git commit\
   echo directly, but instead do one of the following: 
 diff --git a/git-rebase.sh b/git-rebase.sh
 index 842d7d4..055af1b 100755
 --- a/git-rebase.sh
 +++ b/git-rebase.sh
 @@ -37,6 +37,7 @@ ignore-date!   passed to 'git am'
  whitespace=!   passed to 'git apply'
  ignore-whitespace! passed to 'git apply'
  C=!passed to 'git apply'
 +S,gpg-sign?GPG-sign commits
   Actions:
  continue!  continue
  abort! abort and check out the original branch
 @@ -85,6 +86,7 @@ preserve_merges=
  autosquash=
  keep_empty=
  test $(git config --bool rebase.autosquash) = true  autosquash=t
 +gpg_sign_opt=
  
  read_basic_state () {
   test -f $state_dir/head-name 
 @@ -107,6 +109,8 @@ read_basic_state () {
   strategy_opts=$(cat $state_dir/strategy_opts)
   test -f $state_dir/allow_rerere_autoupdate 
   allow_rerere_autoupdate=$(cat 
 $state_dir/allow_rerere_autoupdate)
 + test -f $state_dir/gpg_sign_opt 
 + gpg_sign_opt=$(cat $state_dir/gpg_sign_opt)
  }
  
  write_basic_state () {
 @@ -120,6 +124,7 @@ write_basic_state () {
   $state_dir/strategy_opts
   test -n $allow_rerere_autoupdate  echo $allow_rerere_autoupdate  
 \
   $state_dir/allow_rerere_autoupdate
 + test -n $gpg_sign_opt  echo $gpg_sign_opt  
 $state_dir/gpg_sign_opt
  }
  
  output () {
 @@ -324,6 +329,15 @@ do
   --rerere-autoupdate|--no-rerere-autoupdate)
   allow_rerere_autoupdate=$1
   ;;
 + --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