[PATCH v3 0/9] add --gpg-sign to rebase and pull

[PATCH v3 0/9] add --gpg-sign to rebase and pull

[brian m. carlson]

This series was posted to the list some time back.  This is a re-send of
Nicolas Vigier's work with an additional patch that adds --gpg-sign to
pull as well, as well as the fixes that Junio suggested in review.

Since the parsed option value for --gpg-sign=<value> is used not only as
an argument to other commands, but also as output to the user, I chose
to abuse git rev-parse --sq-quote slightly to quote only the argument,
which I feel is more aesthetically pleasing than quoting the entire
option, and perhaps less confusing for the novice shell user.  Both
options are equally functional.

Other than that, this is a simple reroll addressing Junio's review
comments.  I've been using it on my laptop for some time, and it works
fine for me, although further comments are certainly welcome.

Nicolas Vigier (8):
  cherry-pick, revert: add the --gpg-sign option
  git-sh-setup.sh: add variable to use the stuck-long mode
  am: parse options in stuck-long mode
  am: add the --gpg-sign option
  rebase: remove useless arguments check
  rebase: don't try to match -M option
  rebase: parse options in stuck-long mode
  rebase: add the --gpg-sign option

brian m. carlson (1):
  pull: add the --gpg-sign option.

 Documentation/git-am.txt          |  6 +++-
 Documentation/git-cherry-pick.txt |  7 ++++-
 Documentation/git-rebase.txt      |  4 +++
 Documentation/git-revert.txt      |  6 +++-
 builtin/revert.c                  |  2 ++
 contrib/git-resurrect.sh          |  1 +
 git-am.sh                         | 26 ++++++++++------
 git-instaweb.sh                   |  1 +
 git-pull.sh                       | 13 +++++++-
 git-quiltimport.sh                |  1 +
 git-rebase--am.sh                 |  8 +++--
 git-rebase--interactive.sh        | 32 +++++++++++--------
 git-rebase--merge.sh              |  2 +-
 git-rebase.sh                     | 65 ++++++++++++++++++++++-----------------
 git-request-pull.sh               |  1 +
 git-sh-setup.sh                   |  2 ++
 sequencer.c                       | 11 +++++++
 sequencer.h                       |  2 ++
 18 files changed, 132 insertions(+), 58 deletions(-)

-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 1/9] cherry-pick, revert: add the --gpg-sign option

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-cherry-pick.txt |  7 ++++++-
 Documentation/git-revert.txt      |  6 +++++-
 builtin/revert.c                  |  2 ++
 sequencer.c                       | 11 +++++++++++
 sequencer.h                       |  2 ++
 5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index c205d23..f1e6b2f 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -8,7 +8,8 @@ git-cherry-pick - Apply the changes introduced by some existing commits
 SYNOPSIS
 --------
 [verse]
-'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
+'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff]
+		  [-S[<keyid>]] <commit>...
 'git cherry-pick' --continue
 'git cherry-pick' --quit
 'git cherry-pick' --abort
@@ -100,6 +101,10 @@ effect to your index in a row.
 --signoff::
 	Add Signed-off-by line at the end of the commit message.
 
+-S[<keyid>]::
+--gpg-sign[=<keyid>]::
+	GPG-sign commits.
+
 --ff::
 	If the current HEAD is the same as the parent of the
 	cherry-pick'ed commit, then a fast forward to this commit will
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 2de67a5..9eb83f0 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -8,7 +8,7 @@ git-revert - Revert some existing commits
 SYNOPSIS
 --------
 [verse]
-'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...
+'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<keyid>]] <commit>...
 'git revert' --continue
 'git revert' --quit
 'git revert' --abort
@@ -80,6 +80,10 @@ more details.
 This is useful when reverting more than one commits'
 effect to your index in a row.
 
+-S[<keyid>]::
+--gpg-sign[=<keyid>]::
+	GPG-sign commits.
+
 -s::
 --signoff::
 	Add Signed-off-by line at the end of the commit message.
diff --git a/builtin/revert.c b/builtin/revert.c
index 87659c9..065d88d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -89,6 +89,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),
 		OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
 			N_("option for merge strategy"), option_parse_x),
+		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key id"),
+		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
diff --git a/sequencer.c b/sequencer.c
index 90cac7b..bde5f04 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -392,11 +392,18 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 {
 	struct argv_array array;
 	int rc;
+	char *gpg_sign;
 
 	argv_array_init(&array);
 	argv_array_push(&array, "commit");
 	argv_array_push(&array, "-n");
 
+	if (opts->gpg_sign) {
+		gpg_sign = xmalloc(3 + strlen(opts->gpg_sign));
+		sprintf(gpg_sign, "-S%s", opts->gpg_sign);
+		argv_array_push(&array, gpg_sign);
+		free(gpg_sign);
+	}
 	if (opts->signoff)
 		argv_array_push(&array, "-s");
 	if (!opts->edit) {
@@ -808,6 +815,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 		opts->mainline = git_config_int(key, value);
 	else if (!strcmp(key, "options.strategy"))
 		git_config_string(&opts->strategy, key, value);
+	else if (!strcmp(key, "options.gpg-sign"))
+		git_config_string(&opts->gpg_sign, key, value);
 	else if (!strcmp(key, "options.strategy-option")) {
 		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
 		opts->xopts[opts->xopts_nr++] = xstrdup(value);
@@ -981,6 +990,8 @@ static void save_opts(struct replay_opts *opts)
 	}
 	if (opts->strategy)
 		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+	if (opts->gpg_sign)
+		git_config_set_in_file(opts_file, "options.gpg-sign", opts->gpg_sign);
 	if (opts->xopts) {
 		int i;
 		for (i = 0; i < opts->xopts_nr; i++)
diff --git a/sequencer.h b/sequencer.h
index 1fc22dc..db43e9c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -37,6 +37,8 @@ struct replay_opts {
 
 	int mainline;
 
+	const char *gpg_sign;
+
 	/* Merge strategy */
 	const char *strategy;
 	const char **xopts;
-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 2/9] git-sh-setup.sh: add variable to use the stuck-long mode

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

If the variable $OPTIONS_STUCKLONG is not empty, then rev-parse
option parsing is done in --stuck-long mode.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 contrib/git-resurrect.sh | 1 +
 git-am.sh                | 1 +
 git-instaweb.sh          | 1 +
 git-quiltimport.sh       | 1 +
 git-rebase.sh            | 1 +
 git-request-pull.sh      | 1 +
 git-sh-setup.sh          | 2 ++
 7 files changed, 8 insertions(+)

diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh
index a4ed4c3..d7e97bb 100755
--- a/contrib/git-resurrect.sh
+++ b/contrib/git-resurrect.sh
@@ -10,6 +10,7 @@ is rather slow but allows you to resurrect other people's topic
 branches."
 
 OPTIONS_KEEPDASHDASH=
+OPTIONS_STUCKLONG=
 OPTIONS_SPEC="\
 git resurrect $USAGE
 --
diff --git a/git-am.sh b/git-am.sh
index bbea430..a3b6f98 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -4,6 +4,7 @@
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
+OPTIONS_STUCKLONG=
 OPTIONS_SPEC="\
 git am [options] [(<mbox>|<Maildir>)...]
 git am [options] (--continue | --skip | --abort)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index e93a238..4aa3eb8 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -5,6 +5,7 @@
 
 PERL='@@PERL@@'
 OPTIONS_KEEPDASHDASH=
+OPTIONS_STUCKLONG=
 OPTIONS_SPEC="\
 git instaweb [options] (--start | --stop | --restart)
 --
diff --git a/git-quiltimport.sh b/git-quiltimport.sh
index 8e17525..167d79f 100755
--- a/git-quiltimport.sh
+++ b/git-quiltimport.sh
@@ -1,5 +1,6 @@
 #!/bin/sh
 OPTIONS_KEEPDASHDASH=
+OPTIONS_STUCKLONG=
 OPTIONS_SPEC="\
 git quiltimport [options]
 --
diff --git a/git-rebase.sh b/git-rebase.sh
index 8a3efa2..c1f98ae 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -5,6 +5,7 @@
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
+OPTIONS_STUCKLONG=
 OPTIONS_SPEC="\
 git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] [<upstream>] [<branch>]
 git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] --root [<branch>]
diff --git a/git-request-pull.sh b/git-request-pull.sh
index fe21d5d..cf4f150 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -9,6 +9,7 @@ LONG_USAGE='Summarizes the changes between two commits to the standard output,
 and includes the given URL in the generated summary.'
 SUBDIRECTORY_OK='Yes'
 OPTIONS_KEEPDASHDASH=
+OPTIONS_STUCKLONG=
 OPTIONS_SPEC='git request-pull [options] start url [end]
 --
 p    show patch text as well
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index fffa3c7..5f28b32 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -72,6 +72,8 @@ if test -n "$OPTIONS_SPEC"; then
 	parseopt_extra=
 	[ -n "$OPTIONS_KEEPDASHDASH" ] &&
 		parseopt_extra="--keep-dashdash"
+	[ -n "$OPTIONS_STUCKLONG" ] &&
+		parseopt_extra="$parseopt_extra --stuck-long"
 
 	eval "$(
 		echo "$OPTIONS_SPEC" |
-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 3/9] am: parse options in stuck-long mode

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

There is no functional change. The reason for this change is to be able
to add a new option taking an optional argument.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-am.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index a3b6f98..020abf6 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -4,7 +4,7 @@
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
-OPTIONS_STUCKLONG=
+OPTIONS_STUCKLONG=t
 OPTIONS_SPEC="\
 git am [options] [(<mbox>|<Maildir>)...]
 git am [options] (--continue | --skip | --abort)
@@ -414,14 +414,14 @@ it will be removed. Please do not use it anymore."
 		abort=t ;;
 	--rebasing)
 		rebasing=t threeway=t ;;
-	--resolvemsg)
-		shift; resolvemsg=$1 ;;
-	--whitespace|--directory|--exclude|--include)
-		git_apply_opt="$git_apply_opt $(sq "$1=$2")"; shift ;;
-	-C|-p)
-		git_apply_opt="$git_apply_opt $(sq "$1$2")"; shift ;;
-	--patch-format)
-		shift ; patch_format="$1" ;;
+	--resolvemsg=*)
+		resolvemsg="${1#--resolvemsg=}" ;;
+	--whitespace=*|--directory=*|--exclude=*|--include=*)
+		git_apply_opt="$git_apply_opt $(sq "$1")" ;;
+	-C*|-p*)
+		git_apply_opt="$git_apply_opt $(sq "$1")" ;;
+	--patch-format=*)
+		patch_format="${1#--patch-format=}" ;;
 	--reject|--ignore-whitespace|--ignore-space-change)
 		git_apply_opt="$git_apply_opt $1" ;;
 	--committer-date-is-author-date)
-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 4/9] am: add the --gpg-sign option

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-am.txt | 6 +++++-
 git-am.sh                | 9 ++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 54d8461..17924d0 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 	 [--ignore-date] [--ignore-space-change | --ignore-whitespace]
 	 [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
 	 [--exclude=<path>] [--include=<path>] [--reject] [-q | --quiet]
-	 [--[no-]scissors]
+	 [--[no-]scissors] [-S[<keyid>]]
 	 [(<mbox> | <Maildir>)...]
 'git am' (--continue | --skip | --abort)
 
@@ -119,6 +119,10 @@ default.   You can use `--no-utf8` to override this.
 	Skip the current patch.  This is only meaningful when
 	restarting an aborted patch.
 
+-S[<keyid>]::
+--gpg-sign[=<keyid>]::
+	GPG-sign commits.
+
 --continue::
 -r::
 --resolved::
diff --git a/git-am.sh b/git-am.sh
index 020abf6..78517f2 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -38,6 +38,7 @@ abort           restore the original branch and abort the patching operation.
 committer-date-is-author-date    lie about committer date
 ignore-date     use current timestamp for author date
 rerere-autoupdate update the index with reused conflict resolution if possible
+S,gpg-sign?     GPG-sign commits
 rebasing*       (internal use for git-rebase)"
 
 . git-sh-setup
@@ -375,6 +376,7 @@ git_apply_opt=
 committer_date_is_author_date=
 ignore_date=
 allow_rerere_autoupdate=
+gpg_sign_opt=
 
 if test "$(git config --bool --get am.keepcr)" = true
 then
@@ -436,6 +438,10 @@ it will be removed. Please do not use it anymore."
 		keepcr=t ;;
 	--no-keep-cr)
 		keepcr=f ;;
+	--gpg-sign)
+		gpg_sign_opt=-S ;;
+	--gpg-sign=*)
+		gpg_sign_opt="-S${1#--gpg-sign=}" ;;
 	--)
 		shift; break ;;
 	*)
@@ -900,7 +906,8 @@ did you forget to use 'git add'?"
 			GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE"
 			export GIT_COMMITTER_DATE
 		fi &&
-		git commit-tree $tree ${parent:+-p} $parent <"$dotest/final-commit"
+		git commit-tree ${parent:+-p} $parent ${gpg_sign_opt:+"$gpg_sign_opt"} $tree  \
+			<"$dotest/final-commit"
 	) &&
 	git update-ref -m "$GIT_REFLOG_ACTION: $FIRSTLINE" HEAD $commit $parent ||
 	stop_here $this
-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 5/9] rebase: remove useless arguments check

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

Remove a check on the number of arguments for --onto and -x options.
It is not possible for $# to be <= 2 at this point :

 - if --onto or -x has an argument, git rev-parse --parseopt will
   provide something like this :
     set -- --onto 'x' --
   when parsing the "--onto" option, $# will be 3 or more if there are
   other options.

 - if --onto or -x doesn't have an argument, git rev-parse --parseopt
   will exit with an error and display usage information.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-rebase.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index c1f98ae..d1835ba 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -238,12 +238,10 @@ do
 		action=${1##--}
 		;;
 	--onto)
-		test 2 -le "$#" || usage
 		onto="$2"
 		shift
 		;;
 	-x)
-		test 2 -le "$#" || usage
 		cmd="${cmd}exec $2${LF}"
 		shift
 		;;
-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 6/9] rebase: don't try to match -M option

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

The -M option does not exist in OPTIONS_SPEC, so there is no use to try
to find it.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index d1835ba..3b55211 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -267,7 +267,7 @@ do
 	--no-fork-point)
 		fork_point=
 		;;
-	-M|-m)
+	-m)
 		do_merge=t
 		;;
 	-X)
-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 7/9] rebase: parse options in stuck-long mode

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

There is no functionnal change. The reason for this change is to be able
to add a new option taking an optional argument.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-rebase.sh | 50 ++++++++++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 3b55211..842d7d4 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -5,7 +5,7 @@
 
 SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
-OPTIONS_STUCKLONG=
+OPTIONS_STUCKLONG=t
 OPTIONS_SPEC="\
 git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] [<upstream>] [<branch>]
 git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] --root [<branch>]
@@ -237,21 +237,19 @@ do
 		test $total_argc -eq 2 || usage
 		action=${1##--}
 		;;
-	--onto)
-		onto="$2"
-		shift
+	--onto=*)
+		onto="${1#--onto=}"
 		;;
-	-x)
-		cmd="${cmd}exec $2${LF}"
-		shift
+	--exec=*)
+		cmd="${cmd}exec ${1#--exec=}${LF}"
 		;;
-	-i)
+	--interactive)
 		interactive_rebase=explicit
 		;;
-	-k)
+	--keep-empty)
 		keep_empty=yes
 		;;
-	-p)
+	--preserve-merges)
 		preserve_merges=t
 		test -z "$interactive_rebase" && interactive_rebase=implied
 		;;
@@ -267,21 +265,19 @@ do
 	--no-fork-point)
 		fork_point=
 		;;
-	-m)
+	--merge)
 		do_merge=t
 		;;
-	-X)
-		shift
-		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--$1")"
+	--strategy-option=*)
+		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--${1#--strategy-option=}")"
 		do_merge=t
 		test -z "$strategy" && strategy=recursive
 		;;
-	-s)
-		shift
-		strategy="$1"
+	--strategy=*)
+		strategy="${1#--strategy=}"
 		do_merge=t
 		;;
-	-n)
+	--no-stat)
 		diffstat=
 		;;
 	--stat)
@@ -290,21 +286,20 @@ do
 	--autostash)
 		autostash=true
 		;;
-	-v)
+	--verbose)
 		verbose=t
 		diffstat=t
 		GIT_QUIET=
 		;;
-	-q)
+	--quiet)
 		GIT_QUIET=t
 		git_am_opt="$git_am_opt -q"
 		verbose=
 		diffstat=
 		;;
-	--whitespace)
-		shift
-		git_am_opt="$git_am_opt --whitespace=$1"
-		case "$1" in
+	--whitespace=*)
+		git_am_opt="$git_am_opt --whitespace=${1#--whitespace=}"
+		case "${1#--whitespace=}" in
 		fix|strip)
 			force_rebase=t
 			;;
@@ -317,14 +312,13 @@ do
 		git_am_opt="$git_am_opt $1"
 		force_rebase=t
 		;;
-	-C)
-		shift
-		git_am_opt="$git_am_opt -C$1"
+	-C*)
+		git_am_opt="$git_am_opt $1"
 		;;
 	--root)
 		rebase_root=t
 		;;
-	-f|--no-ff)
+	--force-rebase|--no-ff)
 		force_rebase=t
 		;;
 	--rerere-autoupdate|--no-rerere-autoupdate)
-- 
1.9.rc1.1006.g13f506b.dirty

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

[brian m. carlson]

From: Nicolas Vigier <boklm@mars-attacks.org>

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-rebase.txt |  4 ++++
 git-rebase--am.sh            |  8 +++++---
 git-rebase--interactive.sh   | 32 ++++++++++++++++++++------------
 git-rebase--merge.sh         |  2 +-
 git-rebase.sh                | 14 ++++++++++++++
 5 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 2889be6..2a93c64 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -281,6 +281,10 @@ which makes little sense.
 	specified, `-s recursive`.  Note the reversal of 'ours' and
 	'theirs' as noted above for the `-m` option.
 
+-S[<keyid>]::
+--gpg-sign[=<keyid>]::
+	GPG-sign commits.
+
 -q::
 --quiet::
 	Be quiet. Implies --no-stat.
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index a4f683a..df46f4c 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -6,7 +6,8 @@
 
 case "$action" in
 continue)
-	git am --resolved --resolvemsg="$resolvemsg" &&
+	git am --resolved --resolvemsg="$resolvemsg" \
+		${gpg_sign_opt:+"$gpg_sign_opt"} &&
 	move_to_original_branch
 	return
 	;;
@@ -26,7 +27,7 @@ then
 	# empty commits and even if it didn't the format doesn't really lend
 	# itself well to recording empty patches.  fortunately, cherry-pick
 	# makes this easy
-	git cherry-pick --allow-empty "$revisions"
+	git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty "$revisions"
 	ret=$?
 else
 	rm -f "$GIT_DIR/rebased-patches"
@@ -60,7 +61,8 @@ else
 		return $?
 	fi
 
-	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
+	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" \
+		${gpg_sign_opt:+"$gpg_sign_opt"} <"$GIT_DIR/rebased-patches"
 	ret=$?
 
 	rm -f "$GIT_DIR/rebased-patches"
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 "$@"
 }
 
 pick_one_preserving_merges () {
@@ -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"} \
+				"$strategy_args" "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac
@@ -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"} ||
 			die_with_patch $1 "Could not apply $1... $2"
 	else
 		pick_one $1 ||
@@ -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"} || {
 			warn "Could not amend commit after successfully picking $sha1... $rest"
 			warn "This is most likely due to an empty commit message, or the pre-commit hook"
 			warn "failed. If the pre-commit hook failed, you may need to resolve the issue before"
@@ -542,19 +545,22 @@ do_next () {
 		squash|s|fixup|f)
 			# This is an intermediate commit; its message will only be
 			# used in case of trouble.  So use the long version:
-			do_with_author output git commit --amend --no-verify -F "$squash_msg" ||
+			do_with_author output git commit --amend --no-verify -F "$squash_msg" \
+				${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				die_failed_squash $sha1 "$rest"
 			;;
 		*)
 			# This is the final command of this squash/fixup group
 			if test -f "$fixup_msg"
 			then
-				do_with_author git commit --amend --no-verify -F "$fixup_msg" ||
+				do_with_author git commit --amend --no-verify -F "$fixup_msg" \
+					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
 			else
 				cp "$squash_msg" "$GIT_DIR"/SQUASH_MSG || exit
 				rm -f "$GIT_DIR"/MERGE_MSG
-				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e ||
+				do_with_author git commit --amend --no-verify -F "$GIT_DIR"/SQUASH_MSG -e \
+					${gpg_sign_opt:+"$gpg_sign_opt"} ||
 					die_failed_squash $sha1 "$rest"
 			fi
 			rm -f "$squash_msg" "$fixup_msg"
@@ -822,11 +828,11 @@ continue)
 			die "You have staged changes in your working tree. If these changes are meant to be
 squashed into the previous commit, run:
 
-  git commit --amend
+  git commit --amend $gpg_sign_opt
 
 If they are meant to go into a new commit, run:
 
-  git commit
+  git commit $gpg_sign_opt
 
 In both case, once you're done, continue with:
 
@@ -842,10 +848,12 @@ In both case, once you're done, continue with:
 			die "\
 You have uncommitted changes in your working tree. Please, commit them
 first and then run 'git rebase --continue' again."
-			do_with_author git commit --amend --no-verify -F "$msg" -e ||
+			do_with_author git commit --amend --no-verify -F "$msg" -e \
+				${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				die "Could not commit staged changes."
 		else
-			do_with_author git commit --no-verify -F "$msg" -e ||
+			do_with_author git commit --no-verify -F "$msg" -e \
+				${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				die "Could not commit staged changes."
 		fi
 	fi
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"
 		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/^ //')"
+		;;
 	--)
 		shift
 		break
-- 
1.9.rc1.1006.g13f506b.dirty

[PATCH v3 9/9] pull: add the --gpg-sign option.

[brian m. carlson]

git merge already allows us to sign commits, and git rebase has recently
learned how to do so as well.  Teach git pull to parse the -S/--gpg-sign
option and pass this along to merge or rebase, as appropriate.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 git-pull.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/git-pull.sh b/git-pull.sh
index 0a5aa2c..3b2ea9e 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -138,6 +138,15 @@ do
 	--no-verify-signatures)
 		verify_signatures=--no-verify-signatures
 		;;
+	--gpg-sign|-S)
+		gpg_sign_args=-S
+		;;
+	--gpg-sign=*)
+		gpg_sign_args="$(git rev-parse --sq-quote "-S${1#--gpg-sign=}")"
+		;;
+	-S*)
+		gpg_sign_args="-S${1#-S}"
+		;;
 	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
 		dry_run=--dry-run
 		;;
@@ -305,11 +314,13 @@ merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 case "$rebase" in
 true)
 	eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args $verbosity"
+	eval="$eval $gpg_sign_args"
 	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
 	;;
 *)
 	eval="git-merge $diffstat $no_commit $verify_signatures $edit $squash $no_ff $ff_only"
-	eval="$eval  $log_arg $strategy_args $merge_args $verbosity $progress"
+	eval="$eval $log_arg $strategy_args $merge_args $verbosity $progress"
+	eval="$eval $gpg_sign_args"
 	eval="$eval \"\$merge_name\" HEAD $merge_head"
 	;;
 esac
-- 
1.9.rc1.1006.g13f506b.dirty

Re: [PATCH v3 9/9] pull: add the --gpg-sign option.

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> git merge already allows us to sign commits, and git rebase has recently
> learned how to do so as well.  Teach git pull to parse the -S/--gpg-sign
> option and pass this along to merge or rebase, as appropriate.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  git-pull.sh | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/git-pull.sh b/git-pull.sh
> index 0a5aa2c..3b2ea9e 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -138,6 +138,15 @@ do
>  	--no-verify-signatures)
>  		verify_signatures=--no-verify-signatures
>  		;;
> +	--gpg-sign|-S)
> +		gpg_sign_args=-S
> +		;;
> +	--gpg-sign=*)
> +		gpg_sign_args="$(git rev-parse --sq-quote "-S${1#--gpg-sign=}")"
> +		;;
> +	-S*)
> +		gpg_sign_args="-S${1#-S}"
> +		;;

Interesting.  Remove -S from the beginning and then prefix that with -S?

Also, --gpg-sign='"b m c" <s@c.n>' and -S'"b m c" <s@c.n>' from the
command line of this program would result in $gpg_sign_args that are
differently quoted.  Both of them cannot be correct at the same time.


>  	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
>  		dry_run=--dry-run
>  		;;
> @@ -305,11 +314,13 @@ merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
>  case "$rebase" in
>  true)
>  	eval="git-rebase $diffstat $strategy_args $merge_args $rebase_args $verbosity"
> +	eval="$eval $gpg_sign_args"
>  	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
>  	;;
>  *)
>  	eval="git-merge $diffstat $no_commit $verify_signatures $edit $squash $no_ff $ff_only"
> -	eval="$eval  $log_arg $strategy_args $merge_args $verbosity $progress"
> +	eval="$eval $log_arg $strategy_args $merge_args $verbosity $progress"
> +	eval="$eval $gpg_sign_args"
>  	eval="$eval \"\$merge_name\" HEAD $merge_head"
>  	;;
>  esac

Re: [PATCH v3 1/9] cherry-pick, revert: add the --gpg-sign option

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/sequencer.c b/sequencer.c
> index 90cac7b..bde5f04 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -392,11 +392,18 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  {
>  	struct argv_array array;
>  	int rc;
> +	char *gpg_sign;
>  
>  	argv_array_init(&array);
>  	argv_array_push(&array, "commit");
>  	argv_array_push(&array, "-n");
>  
> +	if (opts->gpg_sign) {
> +		gpg_sign = xmalloc(3 + strlen(opts->gpg_sign));
> +		sprintf(gpg_sign, "-S%s", opts->gpg_sign);
> +		argv_array_push(&array, gpg_sign);
> +		free(gpg_sign);

Perhaps

	argv_array_pushf(&array, "-S%s", opts->gpg_sign);

without any temporary?  That would save 5 lines in total.

> +	}
>  	if (opts->signoff)
>  		argv_array_push(&array, "-s");
>  	if (!opts->edit) {


diff --git a/sequencer.c b/sequencer.c
index bde5f04..b200dce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -392,18 +392,13 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 {
 	struct argv_array array;
 	int rc;
-	char *gpg_sign;
 
 	argv_array_init(&array);
 	argv_array_push(&array, "commit");
 	argv_array_push(&array, "-n");
 
-	if (opts->gpg_sign) {
-		gpg_sign = xmalloc(3 + strlen(opts->gpg_sign));
-		sprintf(gpg_sign, "-S%s", opts->gpg_sign);
-		argv_array_push(&array, gpg_sign);
-		free(gpg_sign);
-	}
+	if (opts->gpg_sign)
+		argv_array_pushf(&array, "-S%s", opts->gpg_sign);
 	if (opts->signoff)
 		argv_array_push(&array, "-s");
 	if (!opts->edit) {

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

[Junio C Hamano]

"brian m. carlson" <sandals@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" <sandals@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='-S"brian m. carson" <sandals@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 "${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.

	echo "run this command: git commit$gpg_sign_opt"


> +		;;
>  	--)
>  		shift
>  		break

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

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

>