[PATCH 0/7] Flags and config to sign pushes by default

[PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

Remembering to pass --signed to git push on every push is extra typing that is
easy to forget, and just leads to annoyance if the remote has a hook that makes
signed pushes required. Add a config option push.gpgSign, analogous to
commit.gpgSign, allowing users to set this flag by default.

Since --signed push will simply fail on any remote that does not advertise a
push cert nonce, actually setting this to true is not very useful (except for
the super-paranoid who would never want to push to a server that does not
support signed pushes). So, add a third state to this boolean, "if-possible",
to sign the push if and only if supported by the server. To keep parity between
the config and command line options, add a --signed-if-possible flag to git
push as well.

The "if-possible" name and weird tri-state boolean is basically a straw man,
and I am happy to change if someone has a clearer suggestion.

Dave Borowitz (7):
  Documentation/git-push.txt: Document when --signed may fail
  Documentation/git-send-pack.txt: Flow long synopsis line
  Documentation/git-send-pack.txt: Document --signed
  gitremote-helpers.txt: Document pushcert option
  transport: Remove git_transport_options.push_cert
  Support signing pushes iff the server supports it
  Add a config option push.gpgSign for default signed pushes

 Documentation/config.txt            |  8 ++++++++
 Documentation/git-push.txt          | 11 +++++++++--
 Documentation/git-send-pack.txt     | 17 ++++++++++++++++-
 Documentation/gitremote-helpers.txt |  3 +++
 builtin/push.c                      | 26 +++++++++++++++++++++++++-
 builtin/send-pack.c                 | 33 +++++++++++++++++++++++++++++++--
 remote-curl.c                       | 14 ++++++++++----
 send-pack.c                         | 18 +++++++++++++++---
 send-pack.h                         |  8 +++++++-
 transport-helper.c                  | 34 +++++++++++++++++-----------------
 transport.c                         | 11 +++++++----
 transport.h                         |  6 +++---
 12 files changed, 151 insertions(+), 38 deletions(-)

-- 
2.5.0.276.gf5e568e

[PATCH 1/7] Documentation/git-push.txt: Document when --signed may fail

[Dave Borowitz]

Like --atomic, --signed will fail if the server does not advertise the
necessary capability. In addition, it requires gpg on the client side.

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Documentation/git-push.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 135d810..f8b8b8b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -137,7 +137,9 @@ already exists on the remote side.
 	GPG-sign the push request to update refs on the receiving
 	side, to allow it to be checked by the hooks and/or be
 	logged.  See linkgit:git-receive-pack[1] for the details
-	on the receiving end.
+	on the receiving end.  If the `gpg` executable is not available,
+	or if the server does not support signed pushes, the push will
+	fail.
 
 --[no-]atomic::
 	Use an atomic transaction on the remote side if available.
-- 
2.5.0.276.gf5e568e

Re: [PATCH 1/7] Documentation/git-push.txt: Document when --signed may fail

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Like --atomic, --signed will fail if the server does not advertise the
> necessary capability. In addition, it requires gpg on the client side.
>
> Signed-off-by: Dave Borowitz <dborowitz@google.com>
> ---
>  Documentation/git-push.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 135d810..f8b8b8b 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -137,7 +137,9 @@ already exists on the remote side.
>  	GPG-sign the push request to update refs on the receiving
>  	side, to allow it to be checked by the hooks and/or be
>  	logged.  See linkgit:git-receive-pack[1] for the details
> -	on the receiving end.
> +	on the receiving end.  If the `gpg` executable is not available,
> +	or if the server does not support signed pushes, the push will
> +	fail.

Looks good.

I am wondering if another mode of failure is worth mentioning: `gpg`
available, you have _some_ keys, but signingkey configured does not
match any of the keys.

Note that I said "am wondering", which is very different from "I
think we should also describe".

Thanks.

Re: [PATCH 1/7] Documentation/git-push.txt: Document when --signed may fail

[Dave Borowitz]

On Fri, Aug 14, 2015 at 7:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> Like --atomic, --signed will fail if the server does not advertise the
>> necessary capability. In addition, it requires gpg on the client side.
>>
>> Signed-off-by: Dave Borowitz <dborowitz@google.com>
>> ---
>>  Documentation/git-push.txt | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>> index 135d810..f8b8b8b 100644
>> --- a/Documentation/git-push.txt
>> +++ b/Documentation/git-push.txt
>> @@ -137,7 +137,9 @@ already exists on the remote side.
>>       GPG-sign the push request to update refs on the receiving
>>       side, to allow it to be checked by the hooks and/or be
>>       logged.  See linkgit:git-receive-pack[1] for the details
>> -     on the receiving end.
>> +     on the receiving end.  If the `gpg` executable is not available,
>> +     or if the server does not support signed pushes, the push will
>> +     fail.
>
> Looks good.
>
> I am wondering if another mode of failure is worth mentioning: `gpg`
> available, you have _some_ keys, but signingkey configured does not
> match any of the keys.
>
> Note that I said "am wondering", which is very different from "I
> think we should also describe".

I think we don't need to go down the path of enumerating all possible
ways the operation can fail. There is probably a reasonably concise
way to include more possibilities. How about:

"If the attempt to sign with `gpg` fails, or if the server does not
support signed pushes, the push will fail."

This should cover gpg not being found, gpg being fatally
misconfigured, crazy unexpected pipe closures, etc.

> Thanks.

[PATCH 2/7] Documentation/git-send-pack.txt: Flow long synopsis line

[Dave Borowitz]

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Documentation/git-send-pack.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index b5d09f7..6affff6 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,8 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]
+'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
+		[--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]
 
 DESCRIPTION
 -----------
-- 
2.5.0.276.gf5e568e

[PATCH 3/7] Documentation/git-send-pack.txt: Document --signed

[Dave Borowitz]

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Documentation/git-send-pack.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 6affff6..dde13b0 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -10,7 +10,8 @@ SYNOPSIS
 --------
 [verse]
 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
-		[--verbose] [--thin] [--atomic] [<host>:]<directory> [<ref>...]
+		[--verbose] [--thin] [--atomic] [--signed]
+		[<host>:]<directory> [<ref>...]
 
 DESCRIPTION
 -----------
@@ -68,6 +69,14 @@ be in a separate packet, and the list must end with a flush packet.
 	fails to update then the entire push will fail without changing any
 	refs.
 
+--signed::
+	GPG-sign the push request to update refs on the receiving
+	side, to allow it to be checked by the hooks and/or be
+	logged.  See linkgit:git-receive-pack[1] for the details
+	on the receiving end.  If the `gpg` executable is not available,
+	or if the server does not support signed pushes, the push will
+	fail.
+
 <host>::
 	A remote host to house the repository.  When this
 	part is specified, 'git-receive-pack' is invoked via
-- 
2.5.0.276.gf5e568e

[PATCH 4/7] gitremote-helpers.txt: Document pushcert option

[Dave Borowitz]

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Documentation/gitremote-helpers.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 82e2d15..78e0b27 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -448,6 +448,9 @@ set by Git if the remote helper has the 'option' capability.
 'option update-shallow {'true'|'false'}::
 	Allow to extend .git/shallow if the new refs require it.
 
+'option pushcert {'true'|'false'}::
+	GPG sign pushes.
+
 SEE ALSO
 --------
 linkgit:git-remote[1]
-- 
2.5.0.276.gf5e568e

[PATCH 5/7] transport: Remove git_transport_options.push_cert

[Dave Borowitz]

This field was set in transport_set_option, but never read in the push
code. The push code basically ignores the smart_options field
entirely, and derives its options from the flags arguments to the
push* callbacks. Note that in git_transport_push there are already
several args set from flags that have no corresponding field in
git_transport_options; after this change, push_cert is just like
those.

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 transport.c | 3 ---
 transport.h | 1 -
 2 files changed, 4 deletions(-)

diff --git a/transport.c b/transport.c
index 40692f8..3dd6e30 100644
--- a/transport.c
+++ b/transport.c
@@ -476,9 +476,6 @@ static int set_git_option(struct git_transport_options *opts,
 				die("transport: invalid depth option '%s'", value);
 		}
 		return 0;
-	} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {
-		opts->push_cert = !!value;
-		return 0;
 	}
 	return 1;
 }
diff --git a/transport.h b/transport.h
index 18d2cf8..79190df 100644
--- a/transport.h
+++ b/transport.h
@@ -12,7 +12,6 @@ struct git_transport_options {
 	unsigned check_self_contained_and_connected : 1;
 	unsigned self_contained_and_connected : 1;
 	unsigned update_shallow : 1;
-	unsigned push_cert : 1;
 	int depth;
 	const char *uploadpack;
 	const char *receivepack;
-- 
2.5.0.276.gf5e568e

Re: [PATCH 5/7] transport: Remove git_transport_options.push_cert

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> This field was set in transport_set_option, but never read in the push
> code. The push code basically ignores the smart_options field
> entirely, and derives its options from the flags arguments to the
> push* callbacks. Note that in git_transport_push there are already
> several args set from flags that have no corresponding field in
> git_transport_options; after this change, push_cert is just like
> those.
>
> Signed-off-by: Dave Borowitz <dborowitz@google.com>
> ---

Thanks for cleaning up my mess.

Honestly, to me, the smart transport is always a second-class
citizen (and http walkers are not even citizens ;-)) and any support
of new feature is added as an after-thought once the feature starts
working with the native transport, and that development pattern
clearly shows in a place like this.

>  transport.c | 3 ---
>  transport.h | 1 -
>  2 files changed, 4 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 40692f8..3dd6e30 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -476,9 +476,6 @@ static int set_git_option(struct git_transport_options *opts,
>  				die("transport: invalid depth option '%s'", value);
>  		}
>  		return 0;
> -	} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {
> -		opts->push_cert = !!value;
> -		return 0;
>  	}
>  	return 1;
>  }
> diff --git a/transport.h b/transport.h
> index 18d2cf8..79190df 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -12,7 +12,6 @@ struct git_transport_options {
>  	unsigned check_self_contained_and_connected : 1;
>  	unsigned self_contained_and_connected : 1;
>  	unsigned update_shallow : 1;
> -	unsigned push_cert : 1;
>  	int depth;
>  	const char *uploadpack;
>  	const char *receivepack;

[PATCH 6/7] Support signing pushes iff the server supports it

[Dave Borowitz]

Add a new flag --signed-if-possible to push and send-pack that sends a
push certificate if and only if the server advertised a push cert
nonce. If not, at least warn the user that their push may not be as
secure as they thought.

Signed-off-by: Dave Borowitz <dborowitz@google.com>
---
 Documentation/git-push.txt      |  9 +++++++--
 Documentation/git-send-pack.txt |  9 +++++++--
 builtin/push.c                  |  4 +++-
 builtin/send-pack.c             |  6 +++++-
 remote-curl.c                   | 14 ++++++++++----
 send-pack.c                     | 18 +++++++++++++++---
 send-pack.h                     |  8 +++++++-
 transport-helper.c              | 34 +++++++++++++++++-----------------
 transport.c                     |  8 +++++++-
 transport.h                     |  5 +++--
 10 files changed, 81 insertions(+), 34 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index f8b8b8b..fcfdf73 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose]
-	   [-u | --set-upstream] [--signed]
+	   [-u | --set-upstream] [--signed] [--signed-if-possible]
 	   [--force-with-lease[=<refname>[:<expect>]]]
 	   [--no-verify] [<repository> [<refspec>...]]
 
@@ -139,7 +139,12 @@ already exists on the remote side.
 	logged.  See linkgit:git-receive-pack[1] for the details
 	on the receiving end.  If the `gpg` executable is not available,
 	or if the server does not support signed pushes, the push will
-	fail.
+	fail. Takes precedence over --signed-if-possible.
+
+--signed-if-possible::
+	Like --signed, but only if the server supports signed pushes. If
+	the server supports signed pushes but the `gpg` is not available,
+	the push will fail.
 
 --[no-]atomic::
 	Use an atomic transaction on the remote side if available.
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index dde13b0..5789208 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
-		[--verbose] [--thin] [--atomic] [--signed]
+		[--verbose] [--thin] [--atomic] [--signed] [--signed-if-possible]
 		[<host>:]<directory> [<ref>...]
 
 DESCRIPTION
@@ -75,7 +75,12 @@ be in a separate packet, and the list must end with a flush packet.
 	logged.  See linkgit:git-receive-pack[1] for the details
 	on the receiving end.  If the `gpg` executable is not available,
 	or if the server does not support signed pushes, the push will
-	fail.
+	fail. Takes precedence over --signed-if-possible.
+
+--signed-if-possible::
+	Like --signed, but only if the server supports signed pushes. If
+	the server supports signed pushes but the `gpg` is not available,
+	the push will fail.
 
 <host>::
 	A remote host to house the repository.  When this
diff --git a/builtin/push.c b/builtin/push.c
index 57c138b..95a67c5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -526,7 +526,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
 		OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
 			TRANSPORT_PUSH_FOLLOW_TAGS),
-		OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), TRANSPORT_PUSH_CERT),
+		OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), TRANSPORT_PUSH_CERT_ALWAYS),
+		OPT_BIT(0, "signed-if-possible", &flags, N_("GPG sign the push, if supported by the server"),
+			TRANSPORT_PUSH_CERT_IF_POSSIBLE),
 		OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on remote side"), TRANSPORT_PUSH_ATOMIC),
 		OPT_END()
 	};
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 23b2962..8eebbf4 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -158,7 +158,11 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--signed")) {
-				args.push_cert = 1;
+				args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
+				continue;
+			}
+			if (!strcmp(arg, "--signed-if-possible")) {
+				args.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;
 				continue;
 			}
 			if (!strcmp(arg, "--progress")) {
diff --git a/remote-curl.c b/remote-curl.c
index af7b678..f049de8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -11,6 +11,7 @@
 #include "argv-array.h"
 #include "credential.h"
 #include "sha1-array.h"
+#include "send-pack.h"
 
 static struct remote *remote;
 /* always ends with a trailing slash */
@@ -26,7 +27,8 @@ struct options {
 		followtags : 1,
 		dry_run : 1,
 		thin : 1,
-		push_cert : 1;
+		/* One of the SEND_PACK_PUSH_CERT_* constants. */
+		push_cert : 2;
 };
 static struct options options;
 static struct string_list cas_options = STRING_LIST_INIT_DUP;
@@ -109,9 +111,11 @@ static int set_option(const char *name, const char *value)
 		return 0;
 	} else if (!strcmp(name, "pushcert")) {
 		if (!strcmp(value, "true"))
-			options.push_cert = 1;
+			options.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
 		else if (!strcmp(value, "false"))
-			options.push_cert = 0;
+			options.push_cert = SEND_PACK_PUSH_CERT_NEVER;
+		else if (!strcmp(value, "if-possible"))
+			options.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;
 		else
 			return -1;
 		return 0;
@@ -880,8 +884,10 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 		argv_array_push(&args, "--thin");
 	if (options.dry_run)
 		argv_array_push(&args, "--dry-run");
-	if (options.push_cert)
+	if (options.push_cert == SEND_PACK_PUSH_CERT_ALWAYS)
 		argv_array_push(&args, "--signed");
+	else if (options.push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE)
+		argv_array_push(&args, "--signed-if-possible");
 	if (options.verbosity == 0)
 		argv_array_push(&args, "--quiet");
 	else if (options.verbosity > 1)
diff --git a/send-pack.c b/send-pack.c
index 2a64fec..6ae9f45 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -370,7 +370,7 @@ int send_pack(struct send_pack_args *args,
 		args->use_thin_pack = 0;
 	if (server_supports("atomic"))
 		atomic_supported = 1;
-	if (args->push_cert) {
+	if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
 		int len;
 
 		push_cert_nonce = server_feature_value("push-cert", &len);
@@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,
 		reject_invalid_nonce(push_cert_nonce, len);
 		push_cert_nonce = xmemdupz(push_cert_nonce, len);
 	}
+	if (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {
+		int len;
+
+		push_cert_nonce = server_feature_value("push-cert", &len);
+		if (push_cert_nonce) {
+			reject_invalid_nonce(push_cert_nonce, len);
+			push_cert_nonce = xmemdupz(push_cert_nonce, len);
+		} else
+			warning(_("not sending a push certificate since the"
+				  " receiving end does not support --signed"
+				  " push"));
+	}
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -413,7 +425,7 @@ int send_pack(struct send_pack_args *args,
 	if (!args->dry_run)
 		advertise_shallow_grafts_buf(&req_buf);
 
-	if (!args->dry_run && args->push_cert)
+	if (!args->dry_run && push_cert_nonce)
 		cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
 					       cap_buf.buf, push_cert_nonce);
 
@@ -452,7 +464,7 @@ int send_pack(struct send_pack_args *args,
 	for (ref = remote_refs; ref; ref = ref->next) {
 		char *old_hex, *new_hex;
 
-		if (args->dry_run || args->push_cert)
+		if (args->dry_run || push_cert_nonce)
 			continue;
 
 		if (check_to_send_update(ref, args) < 0)
diff --git a/send-pack.h b/send-pack.h
index b664648..5042b65 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -1,6 +1,11 @@
 #ifndef SEND_PACK_H
 #define SEND_PACK_H
 
+/* Possible values for push_cert field in send_pack_args. */
+#define SEND_PACK_PUSH_CERT_NEVER 0
+#define SEND_PACK_PUSH_CERT_IF_POSSIBLE 1
+#define SEND_PACK_PUSH_CERT_ALWAYS 2
+
 struct send_pack_args {
 	const char *url;
 	unsigned verbose:1,
@@ -12,7 +17,8 @@ struct send_pack_args {
 		use_thin_pack:1,
 		use_ofs_delta:1,
 		dry_run:1,
-		push_cert:1,
+		/* One of the SEND_PACK_PUSH_CERT_* constants. */
+		push_cert:2,
 		stateless_rpc:1,
 		atomic:1;
 };
diff --git a/transport-helper.c b/transport-helper.c
index 5d99a6b..bf4dd22 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -257,7 +257,6 @@ static const char *boolean_options[] = {
 	TRANS_OPT_THIN,
 	TRANS_OPT_KEEP,
 	TRANS_OPT_FOLLOWTAGS,
-	TRANS_OPT_PUSH_CERT
 	};
 
 static int set_helper_option(struct transport *transport,
@@ -763,6 +762,21 @@ static int push_update_refs_status(struct helper_data *data,
 	return ret;
 }
 
+static void set_common_push_options(struct transport *transport,
+				   const char *name, int flags)
+{
+	if (flags & TRANSPORT_PUSH_DRY_RUN) {
+		if (set_helper_option(transport, "dry-run", "true") != 0)
+			die("helper %s does not support dry-run", name);
+	} else if (flags & TRANSPORT_PUSH_CERT_ALWAYS) {
+		if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "true") != 0)
+			die("helper %s does not support --signed", name);
+	} else if (flags & TRANSPORT_PUSH_CERT_IF_POSSIBLE) {
+		if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "if-possible") != 0)
+			die("helper %s does not support --signed-if-possible", name);
+	}
+}
+
 static int push_refs_with_push(struct transport *transport,
 			       struct ref *remote_refs, int flags)
 {
@@ -830,14 +844,7 @@ static int push_refs_with_push(struct transport *transport,
 
 	for_each_string_list_item(cas_option, &cas_options)
 		set_helper_option(transport, "cas", cas_option->string);
-
-	if (flags & TRANSPORT_PUSH_DRY_RUN) {
-		if (set_helper_option(transport, "dry-run", "true") != 0)
-			die("helper %s does not support dry-run", data->name);
-	} else if (flags & TRANSPORT_PUSH_CERT) {
-		if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "true") != 0)
-			die("helper %s does not support --signed", data->name);
-	}
+	set_common_push_options(transport, data->name, flags);
 
 	strbuf_addch(&buf, '\n');
 	sendline(data, &buf);
@@ -858,14 +865,7 @@ static int push_refs_with_export(struct transport *transport,
 	if (!data->refspecs)
 		die("remote-helper doesn't support push; refspec needed");
 
-	if (flags & TRANSPORT_PUSH_DRY_RUN) {
-		if (set_helper_option(transport, "dry-run", "true") != 0)
-			die("helper %s does not support dry-run", data->name);
-	} else if (flags & TRANSPORT_PUSH_CERT) {
-		if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "true") != 0)
-			die("helper %s does not support --signed", data->name);
-	}
-
+	set_common_push_options(transport, data->name, flags);
 	if (flags & TRANSPORT_PUSH_FORCE) {
 		if (set_helper_option(transport, "force", "true") != 0)
 			warning("helper %s does not support 'force'", data->name);
diff --git a/transport.c b/transport.c
index 3dd6e30..66cd2d3 100644
--- a/transport.c
+++ b/transport.c
@@ -826,10 +826,16 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	args.progress = transport->progress;
 	args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
 	args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
-	args.push_cert = !!(flags & TRANSPORT_PUSH_CERT);
 	args.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);
 	args.url = transport->url;
 
+	if (flags & TRANSPORT_PUSH_CERT_ALWAYS)
+		args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
+	else if (flags & TRANSPORT_PUSH_CERT_IF_POSSIBLE)
+		args.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;
+	else
+		args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
+
 	ret = send_pack(&args, data->fd, data->conn, remote_refs,
 			&data->extra_have);
 
diff --git a/transport.h b/transport.h
index 79190df..49f3e44 100644
--- a/transport.h
+++ b/transport.h
@@ -123,8 +123,9 @@ struct transport {
 #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
 #define TRANSPORT_PUSH_NO_HOOK 512
 #define TRANSPORT_PUSH_FOLLOW_TAGS 1024
-#define TRANSPORT_PUSH_CERT 2048
-#define TRANSPORT_PUSH_ATOMIC 4096
+#define TRANSPORT_PUSH_CERT_ALWAYS 2048
+#define TRANSPORT_PUSH_CERT_IF_POSSIBLE 4096
+#define TRANSPORT_PUSH_ATOMIC 8192
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
-- 
2.5.0.276.gf5e568e

Re: [PATCH 6/7] Support signing pushes iff the server supports it

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> diff --git a/send-pack.c b/send-pack.c
> index 2a64fec..6ae9f45 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -370,7 +370,7 @@ int send_pack(struct send_pack_args *args,
>  		args->use_thin_pack = 0;
>  	if (server_supports("atomic"))
>  		atomic_supported = 1;
> -	if (args->push_cert) {
> +	if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
>  		int len;
>  
>  		push_cert_nonce = server_feature_value("push-cert", &len);
> @@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,
>  		reject_invalid_nonce(push_cert_nonce, len);
>  		push_cert_nonce = xmemdupz(push_cert_nonce, len);
>  	}
> +	if (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {
> +		int len;
> +
> +		push_cert_nonce = server_feature_value("push-cert", &len);
> +		if (push_cert_nonce) {
> +			reject_invalid_nonce(push_cert_nonce, len);
> +			push_cert_nonce = xmemdupz(push_cert_nonce, len);
> +		} else
> +			warning(_("not sending a push certificate since the"
> +				  " receiving end does not support --signed"
> +				  " push"));
> +	}

I wonder if the bodies of these two if statements can be a bit
better organized to avoid duplication (I suspect you have tried
and you may already know that the above is the most readable
version, but I haven't tried to do so myself, so...).

Re: [PATCH 6/7] Support signing pushes iff the server supports it

[Dave Borowitz]

On Fri, Aug 14, 2015 at 7:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> diff --git a/send-pack.c b/send-pack.c
>> index 2a64fec..6ae9f45 100644
>> --- a/send-pack.c
>> +++ b/send-pack.c
>> @@ -370,7 +370,7 @@ int send_pack(struct send_pack_args *args,
>>               args->use_thin_pack = 0;
>>       if (server_supports("atomic"))
>>               atomic_supported = 1;
>> -     if (args->push_cert) {
>> +     if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
>>               int len;
>>
>>               push_cert_nonce = server_feature_value("push-cert", &len);
>> @@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,
>>               reject_invalid_nonce(push_cert_nonce, len);
>>               push_cert_nonce = xmemdupz(push_cert_nonce, len);
>>       }
>> +     if (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {
>> +             int len;
>> +
>> +             push_cert_nonce = server_feature_value("push-cert", &len);
>> +             if (push_cert_nonce) {
>> +                     reject_invalid_nonce(push_cert_nonce, len);
>> +                     push_cert_nonce = xmemdupz(push_cert_nonce, len);
>> +             } else
>> +                     warning(_("not sending a push certificate since the"
>> +                               " receiving end does not support --signed"
>> +                               " push"));
>> +     }
>
> I wonder if the bodies of these two if statements can be a bit
> better organized to avoid duplication (I suspect you have tried
> and you may already know that the above is the most readable
> version, but I haven't tried to do so myself, so...).

Found a slightly less repetitious way.

[PATCH 7/7] Add a config option push.gpgSign for default signed pushes

[Dave Borowitz]

---
 Documentation/config.txt |  8 ++++++++
 builtin/push.c           | 22 ++++++++++++++++++++++
 builtin/send-pack.c      | 27 ++++++++++++++++++++++++++-
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 016f6e9..6804f5b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2178,6 +2178,14 @@ push.followTags::
 	may override this configuration at time of push by specifying
 	'--no-follow-tags'.
 
+push.gpgSign::
+	May be set to a boolean value, or the string 'if-possible'. A
+	true value causes all pushes to be GPG signed, as if '--signed'
+	is passed to linkgit:git-push[1]. The string 'if-possible'
+	causes pushes to be signed if the server supports it, as if
+	'--signed-if-possible' is passed to 'git push'. A false value
+	may override a value from a lower-priority config file. An
+	explicit command-line flag always overrides this config option.
 
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
diff --git a/builtin/push.c b/builtin/push.c
index 95a67c5..8972193 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb)
 	return git_default_config(k, v, NULL);
 }
 
+static void set_push_cert_flags_from_config(int *flags)
+{
+	const char *value;
+	/* Ignore config if flags were set from command line. */
+	if (*flags & (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE))
+		return;
+	if (!git_config_get_value("push.gpgsign", &value)) {
+		switch (git_config_maybe_bool("push.gpgsign", value)) {
+		case 1:
+			*flags |= TRANSPORT_PUSH_CERT_ALWAYS;
+			break;
+		default:
+			if (value && !strcmp(value, "if-possible"))
+				*flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE;
+			else
+				die(_("Invalid value for 'push.gpgsign'"));
+		}
+	}
+}
+
 int cmd_push(int argc, const char **argv, const char *prefix)
 {
 	int flags = 0;
@@ -537,6 +557,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 
+	set_push_cert_flags_from_config(&flags);
+
 	if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
 		die(_("--delete is incompatible with --all, --mirror and --tags"));
 	if (deleterefs && argc < 2)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 8eebbf4..9c8b7de 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -92,6 +92,31 @@ static void print_helper_status(struct ref *ref)
 	strbuf_release(&buf);
 }
 
+static int send_pack_config(const char *k, const char *v, void *cb)
+{
+	git_gpg_config(k, v, NULL);
+
+	if (!strcmp(k, "push.gpgsign")) {
+		const char *value;
+		if (!git_config_get_value("push.gpgsign", &value)) {
+			switch (git_config_maybe_bool("push.gpgsign", value)) {
+			case 0:
+				args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
+				break;
+			case 1:
+				args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
+				break;
+			default:
+				if (value && !strcasecmp(value, "if-possible"))
+					args.push_cert = SEND_PACK_PUSH_CERT_IF_POSSIBLE;
+				else
+					return error("Invalid value for '%s'", k);
+			}
+		}
+	}
+	return 0;
+}
+
 int cmd_send_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, nr_refspecs = 0;
@@ -114,7 +139,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int from_stdin = 0;
 	struct push_cas_option cas = {0};
 
-	git_config(git_gpg_config, NULL);
+	git_config(send_pack_config, NULL);
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
-- 
2.5.0.276.gf5e568e

Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> ---

Does the lack of sign-off indicate something (like "this is just a
'what do people think?' weatherbaloon not yet a serious submission")?

> +push.gpgSign::
> +	May be set to a boolean value, or the string 'if-possible'. A
> +	true value causes all pushes to be GPG signed, as if '--signed'
> +	is passed to linkgit:git-push[1]. The string 'if-possible'
> +	causes pushes to be signed if the server supports it, as if
> +	'--signed-if-possible' is passed to 'git push'. A false value
> +	may override a value from a lower-priority config file. An
> +	explicit command-line flag always overrides this config option.

> diff --git a/builtin/push.c b/builtin/push.c
> index 95a67c5..8972193 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb)
>  	return git_default_config(k, v, NULL);
>  }
>  
> +static void set_push_cert_flags_from_config(int *flags)
> +{
> +	const char *value;
> +	/* Ignore config if flags were set from command line. */
> +	if (*flags & (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE))
> +		return;

This looks somewhat strange.  Usually we read from config first and
then from options, so a git_config() callback shouldn't have to
worry about what command line option parser did (because it hasn't
happened yet).  Why isn't the addition to support this new variable
done inside existing git_push_config() callback function?

> +	if (!git_config_get_value("push.gpgsign", &value)) {
> +		switch (git_config_maybe_bool("push.gpgsign", value)) {
> +		case 1:
> +			*flags |= TRANSPORT_PUSH_CERT_ALWAYS;
> +			break;
> +		default:
> +			if (value && !strcmp(value, "if-possible"))
> +				*flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE;
> +			else
> +				die(_("Invalid value for 'push.gpgsign'"));
> +		}
> +	}
> +}
> +

maybe_bool() returns 0 for "false" (and its various spellings), 1
for "true" (and its various spellings) and -1 for "that's not a
bool".

For "A false value may override a value" to be true, we'd need

		case 0:
			*flags &= ~TRANSPORT_PUSH_CERT_ALWAYS;
			break;

or something?

Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes

[Dave Borowitz]

On Mon, Aug 17, 2015 at 1:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> ---
>
> Does the lack of sign-off indicate something (like "this is just a
> 'what do people think?' weatherbaloon not yet a serious submission")?
>
>> +push.gpgSign::
>> +     May be set to a boolean value, or the string 'if-possible'. A
>> +     true value causes all pushes to be GPG signed, as if '--signed'
>> +     is passed to linkgit:git-push[1]. The string 'if-possible'
>> +     causes pushes to be signed if the server supports it, as if
>> +     '--signed-if-possible' is passed to 'git push'. A false value
>> +     may override a value from a lower-priority config file. An
>> +     explicit command-line flag always overrides this config option.
>
>> diff --git a/builtin/push.c b/builtin/push.c
>> index 95a67c5..8972193 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -491,6 +491,26 @@ static int git_push_config(const char *k, const char *v, void *cb)
>>       return git_default_config(k, v, NULL);
>>  }
>>
>> +static void set_push_cert_flags_from_config(int *flags)
>> +{
>> +     const char *value;
>> +     /* Ignore config if flags were set from command line. */
>> +     if (*flags & (TRANSPORT_PUSH_CERT_ALWAYS | TRANSPORT_PUSH_CERT_IF_POSSIBLE))
>> +             return;
>
> This looks somewhat strange.  Usually we read from config first and
> then from options, so a git_config() callback shouldn't have to
> worry about what command line option parser did (because it hasn't
> happened yet).  Why isn't the addition to support this new variable
> done inside existing git_push_config() callback function?

The issue is that if both _ALWAYS and _IF_POSSIBLE are set,
git_transport_push interprets it as _ALWAYS. But, we are also supposed
to prefer explicit command-line options to config values.

Suppose we parsed config first, then options. If the user has
push.signed = always and and passes --signed-if-possible, then the end
result is (_ALWAYS | _IF_POSSIBLE), aka always, and we've violated
"prefer command line options to config values".

I guess the alternative is to have --signed just clear the
_IF_POSSIBLE bit in addition to setting the _ALWAYS bit, and vice
versa for --signed-if-possible. I am not sure what the end result
would be if the user passed a combination of various --signed and
--signed-if-possible flags on the command line; maybe that's not worth
worrying about.

>> +     if (!git_config_get_value("push.gpgsign", &value)) {
>> +             switch (git_config_maybe_bool("push.gpgsign", value)) {
>> +             case 1:
>> +                     *flags |= TRANSPORT_PUSH_CERT_ALWAYS;
>> +                     break;
>> +             default:
>> +                     if (value && !strcmp(value, "if-possible"))
>> +                             *flags |= TRANSPORT_PUSH_CERT_IF_POSSIBLE;
>> +                     else
>> +                             die(_("Invalid value for 'push.gpgsign'"));
>> +             }
>> +     }
>> +}
>> +
>
> maybe_bool() returns 0 for "false" (and its various spellings), 1
> for "true" (and its various spellings) and -1 for "that's not a
> bool".
>
> For "A false value may override a value" to be true, we'd need
>
>                 case 0:
>                         *flags &= ~TRANSPORT_PUSH_CERT_ALWAYS;
>                         break;
>
> or something?

Yes, except unsetting both flags? ~(TRANSPORT_PUSH_CERT_ALWAYS |
TRANSPORT_CERT_IF_POSSIBLE)

Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> The issue is that if both _ALWAYS and _IF_POSSIBLE are set,
> git_transport_push interprets it as _ALWAYS. But, we are also supposed
> to prefer explicit command-line options to config values.
>
> Suppose we parsed config first, then options. If the user has
> push.signed = always and and passes --signed-if-possible, then the end
> result is (_ALWAYS | _IF_POSSIBLE), aka always,...

Doesn't that merely suggest that the option parsing is implemented
incorrectly?  Why is --signed-if-possible just ORing its bits into
the flag, instead of clearing and setting?

Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Dave Borowitz <dborowitz@google.com> writes:
>
>> The issue is that if both _ALWAYS and _IF_POSSIBLE are set,
>> git_transport_push interprets it as _ALWAYS. But, we are also supposed
>> to prefer explicit command-line options to config values.
>>
>> Suppose we parsed config first, then options. If the user has
>> push.signed = always and and passes --signed-if-possible, then the end
>> result is (_ALWAYS | _IF_POSSIBLE), aka always,...
>
> Doesn't that merely suggest that the option parsing is implemented
> incorrectly?  Why is --signed-if-possible just ORing its bits into
> the flag, instead of clearing and setting?

That is, "git config alias.myp push --sign=if-asked" followed by

    $ git myp --sign=no

would internally expand to

    $ git push --sign=if-asked --sign=no

and the result should follow the usual "last one wins" rule.

Re: [PATCH 7/7] Add a config option push.gpgSign for default signed pushes

[Dave Borowitz]

On Mon, Aug 17, 2015 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Dave Borowitz <dborowitz@google.com> writes:
>
> > The issue is that if both _ALWAYS and _IF_POSSIBLE are set,
> > git_transport_push interprets it as _ALWAYS. But, we are also supposed
> > to prefer explicit command-line options to config values.
> >
> > Suppose we parsed config first, then options. If the user has
> > push.signed = always and and passes --signed-if-possible, then the end
> > result is (_ALWAYS | _IF_POSSIBLE), aka always,...
>
> Doesn't that merely suggest that the option parsing is implemented
> incorrectly?  Why is --signed-if-possible just ORing its bits into
> the flag, instead of clearing and setting?

Yes, that would be incorrect, but the actual implementation in my
patch is not incorrect, it just solves the problem a different way.

The alternative I suggested is another correct implementation, and it
sounds like it's more in line with the convention you expect. I'll go
with that.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Chris Packham]

Bike shedding a little (I've never used the signed push functionality)

On Fri, Aug 14, 2015 at 7:00 AM, Dave Borowitz <dborowitz@google.com> wrote:
> The "if-possible" name and weird tri-state boolean is basically a straw man,
> and I am happy to change if someone has a clearer suggestion.

what about git push --signed={always|if-possible} defaulting to
"always" to be backwards compatible. You might also want to add
--no-signed or --signed=no to override your new config option

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Remembering to pass --signed to git push on every push is extra typing that is
> easy to forget, and just leads to annoyance if the remote has a hook that makes
> signed pushes required. Add a config option push.gpgSign, analogous to
> commit.gpgSign, allowing users to set this flag by default.
>
> Since --signed push will simply fail on any remote that does not advertise a
> push cert nonce, actually setting this to true is not very useful (except for
> the super-paranoid who would never want to push to a server that does not
> support signed pushes). So, add a third state to this boolean, "if-possible",
> to sign the push if and only if supported by the server. To keep parity between
> the config and command line options, add a --signed-if-possible flag to git
> push as well.
>
> The "if-possible" name and weird tri-state boolean is basically a straw man,
> and I am happy to change if someone has a clearer suggestion.

Yes, it looks somewhat strange.  Let me go on a slight tangent to
explain why I think it is OK for "push --signed".

First imagine the case where we were talking an optional setting for
"git tag -a" and what our reaction would be.

Because the reason "git tag -s" would fail when "git tag -a" can
succeed can only be because you do not have a working GPG set-up
(i.e. correctly built and installed GPG with a usable key of your
own to sign), I would say "please sign this tag if I can, but do not
bother failing, an unsigned annotated tag is also OK for me" is not
a sensible request.  In such a case, you'd better get your act
together and make yourself ready to sign before doing the "tag -s"
thing.  Otherwise you'd never get around to do it.  So I'd say "git
tag --sign-if-possible" would not make sense.

But "push --signed" can fail even if you have a perfectly good
GPG set-up.  It will not succeed until the receiving end becomes
ready to accept a signed push, and often you would not be in control
of the receiving end.

More importantly, the meaning and the purose of the GPG signature in
signed tags and signed pushes are vastly different.  In the former
case, you are attesting that the signed objects were made by you to
help yourself.  If somebody else created a tag or a commit and
claimed it is from you, you can say "that signature does not match,
it is not mine".

But "signed push" is not about helping you.  It is about helping the
receiving end by allowing them to be more credible when they say
"This is what David Borowitz said he wanted to put at the tip of
this branch" to other people.  Currently, they can only make a weak
claim "Well, you know, the push was made after we authenticated a
pusher with our own authentication methond, and here is the log that
says the pusher was David".  The log entries could be faked, and the
general public cannot audit.  With a signed push, they can say "Here
is the push certificate, dated and signed by David Borowitz", and
the general public can check without trusting the receiving end
(i.e. hosting site).  If the receiving end does not offer signed
pushes, it just means that they are not ready to be helped by you,
and you should have the option of pushing without helping them,
which is what your "if-possible" is about.

Because of the above reasoning, I think a weaker "I want to do a
signed push if the recipient is capable of accepting one, but
otherwise just pushing there is OK" is a perfectly reasonable
request.

So I am fine as long as "if-possible" turns a failure to make signed
push into a success _only_ when the reason of the failure is because
we did not see the capability supported by the receiving end.  If
the reason why you cannot do a signed push is because you cannot
sign push certificate, "if-possible" should still fail.

Thanks.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> So I am fine as long as "if-possible" turns a failure to make signed
> push into a success _only_ when the reason of the failure is because
> we did not see the capability supported by the receiving end.  If
> the reason why you cannot do a signed push is because you cannot
> sign push certificate, "if-possible" should still fail.

I completely agree with your reasoning, and that's exactly how I
implemented and documented --signed-if-possible.

>From patch 6/7:
+--signed-if-possible::
+       Like --signed, but only if the server supports signed pushes. If
+       the server supports signed pushes but the `gpg` is not available,
+       the push will fail.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> The "if-possible" name and weird tri-state boolean is basically a straw man,
>> and I am happy to change if someone has a clearer suggestion.
>
> Yes, it looks somewhat strange.  Let me go on a slight tangent to
> explain why I think it is OK for "push --signed".

I think we agree that there are three possible behaviors for push and
we should allow the user to specify any of the three. The straw-man
strangeness is that two of them are the traditional boolean values
"true/false" and the third is "file not found^W^W^Wif-possible" :)

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Yes, it looks somewhat strange.
> ... The straw-man
> strangeness is that two of them are the traditional boolean values
> "true/false" and the third is "file not found^W^W^Wif-possible" :)

It actually is not uncommon for a Git configuration variable to
start its life as a boolean and then later become tristate (or more)
as we gain experience with the system, so don't worry about it being
"strange".  A tristate, among whose choices two of them are true and
false, is not "strange" around here.

By "strange", I was referring to the possible perception issue on
having a choice other than yes/no for a configuration that allows
you to express your security preference.

Thanks.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

On Fri, Aug 14, 2015 at 4:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> On Fri, Aug 14, 2015 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Yes, it looks somewhat strange.
>> ... The straw-man
>> strangeness is that two of them are the traditional boolean values
>> "true/false" and the third is "file not found^W^W^Wif-possible" :)
>
> It actually is not uncommon for a Git configuration variable to
> start its life as a boolean and then later become tristate (or more)
> as we gain experience with the system, so don't worry about it being
> "strange".  A tristate, among whose choices two of them are true and
> false, is not "strange" around here.

Ok, so let us bikeshed a bit further.

Bikeshed 1.
Option A: --signed/--no-signed--signed-if-possible
Option B: --signed=true|false|if-possible, "--signed" alone implies "=true".

Bikeshed 2.

Option A: if-possible

The possibly confusing thing is one might interpret missing "gpg" to
mean "impossible", i.e. "if gpg is not installed don't attempt to
sign", which is not the behavior we want.

I don't have another succinct way of saying this.
"if-server-supported" is a mouthful. I think Jonathan mentioned
"opportunistic", which is fairly opaque.


> By "strange", I was referring to the possible perception issue on
> having a choice other than yes/no for a configuration that allows
> you to express your security preference.
>
> Thanks.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Ok, so let us bikeshed a bit further.
>
> Bikeshed 1.
> Option A: --signed/--no-signed--signed-if-possible
> Option B: --signed=true|false|if-possible, "--signed" alone implies "=true".
>
> Bikeshed 2.
>
> Option A: if-possible
>
> The possibly confusing thing is one might interpret missing "gpg" to
> mean "impossible", i.e. "if gpg is not installed don't attempt to
> sign", which is not the behavior we want.
>
> I don't have another succinct way of saying this.
> "if-server-supported" is a mouthful. I think Jonathan mentioned
> "opportunistic", which is fairly opaque.

I would call what we agreed is a good behaviour during this
discussion "--sign-if-asked".

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Ok, so let us bikeshed a bit further.
>
> Bikeshed 1.
> Option A: --signed/--no-signed--signed-if-possible
> Option B: --signed=true|false|if-possible, "--signed" alone implies "=true".
>
> Bikeshed 2.
>
> Option A: if-possible
>
> The possibly confusing thing is one might interpret missing "gpg" to
> mean "impossible", i.e. "if gpg is not installed don't attempt to
> sign", which is not the behavior we want.
>
> I don't have another succinct way of saying this.
> "if-server-supported" is a mouthful. I think Jonathan mentioned
> "opportunistic", which is fairly opaque.
>
>> By "strange", I was referring to the possible perception issue on
>> having a choice other than yes/no for a configuration that allows
>> you to express your security preference.

My preference on Bikeshed 1. would probably be to add

    --sign=yes/no/if-asked

and to keep --[no-]signed for "no" and "yes" for existing users.

Regarding Bikeshed 2., I do not have a strong opinion myself.

Thanks.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> Ok, so let us bikeshed a bit further.
>>
>> Bikeshed 1.
>> Option A: --signed/--no-signed--signed-if-possible
>> Option B: --signed=true|false|if-possible, "--signed" alone implies "=true".
>>
>> Bikeshed 2.
>>
>> Option A: if-possible
>>
>> The possibly confusing thing is one might interpret missing "gpg" to
>> mean "impossible", i.e. "if gpg is not installed don't attempt to
>> sign", which is not the behavior we want.
>>
>> I don't have another succinct way of saying this.
>> "if-server-supported" is a mouthful. I think Jonathan mentioned
>> "opportunistic", which is fairly opaque.
>>
>>> By "strange", I was referring to the possible perception issue on
>>> having a choice other than yes/no for a configuration that allows
>>> you to express your security preference.
>
> My preference on Bikeshed 1. would probably be to add
>
>     --sign=yes/no/if-asked
>
> and to keep --[no-]signed for "no" and "yes" for existing users.

Incidentally, I just looked up incidence of true/false vs. yes/no in
command line options, and the results are decidedly undecided:

$ grep -e '--[^ ]*=[^ ]*true' Documentation/*.txt
Documentation/git-init.txt:--shared[=(false|true|umask|group|all|world|everybody|0xxx)]::
Documentation/git-pull.txt:--rebase[=false|true|preserve]::
Documentation/git-svn.txt:--shared[=(false|true|umask|group|all|world|everybody)]::
$ grep -e '--[^ ]*=[^ ]*yes' Documentation/*.txt
Documentation/fetch-options.txt:--recurse-submodules[=yes|on-demand|no]::
Documentation/fetch-options.txt:--recurse-submodules-default=[yes|on-demand]::
Documentation/git-pull.txt:--[no-]recurse-submodules[=yes|on-demand|no]::

Consistency is hard.

I am inclined to stick with yes/no in this case because
--recurse-submodules at least feels like a more modern option that we
should emulate, but don't feel strongly either way.

> Regarding Bikeshed 2., I do not have a strong opinion myself.

Although it sounds like you already expressed an opinion for if-asked
> if-possible, which is stronger than my own :)

> Thanks.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> My preference on Bikeshed 1. would probably be to add
>>
>>     --sign=yes/no/if-asked
>>
>> and to keep --[no-]signed for "no" and "yes" for existing users.
>
> Incidentally, I just looked up incidence of true/false vs. yes/no in
> command line options,...

My yes/no was a short-hand for "yes" (and various other ways to
spell "true") and "no" (and various other ways to spell "false").  I
was NOT bikeshedding to say "I do not like true/false but favor
yes/no".

I actually was expecting a short discussion on sign vs signed,
though.  As "tag --sign" is not "tag --signed" even though we call
the resulting object a "signed tag", "push --sign" may be a good
enough way to spell "signed push".  I _think_ signed pushes are
recent enough that we still have time to deprecate --signed form,
but I do not think it is worth it.

So an updated suggestion would be that we'd take (this is a pretty
much exhaustive enumeration) these:

    --no-signed
    --signed
    --signed=if-asked
    --signed=yes/true/on/1/2...
    --signed=no/false/off/0

We might want to throw in 'always' and 'never' as synonyms for
'true' and 'false', but again I do not think it is worth the
confusion factor, as 'always' and 'true' already mean different
things in some other contexts.

Thanks.

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

On Mon, Aug 17, 2015 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> On Mon, Aug 17, 2015 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> My preference on Bikeshed 1. would probably be to add
>>>
>>>     --sign=yes/no/if-asked
>>>
>>> and to keep --[no-]signed for "no" and "yes" for existing users.
>>
>> Incidentally, I just looked up incidence of true/false vs. yes/no in
>> command line options,...
>
> My yes/no was a short-hand for "yes" (and various other ways to
> spell "true") and "no" (and various other ways to spell "false").  I
> was NOT bikeshedding to say "I do not like true/false but favor
> yes/no".
>
> I actually was expecting a short discussion on sign vs signed,
> though.  As "tag --sign" is not "tag --signed" even though we call
> the resulting object a "signed tag", "push --sign" may be a good
> enough way to spell "signed push".  I _think_ signed pushes are
> recent enough that we still have time to deprecate --signed form,
> but I do not think it is worth it.

I agree that "push --sign" would be better than "push --signed" for
consistency with tag, but will defer to you as to whether it's worth
it to do the deprecation.

> So an updated suggestion would be that we'd take (this is a pretty
> much exhaustive enumeration) these:
>
>     --no-signed
>     --signed
>     --signed=if-asked
>     --signed=yes/true/on/1/2...
>     --signed=no/false/off/0

Fine by me. Would you expect those to all be documented, or just
--[no-]signed|--signed=(yes|no|if-asked) and silently accept the rest?

Is there a common utility function that does what we want? Basically
git_config_maybe_bool but not specifically about configs.

> We might want to throw in 'always' and 'never' as synonyms for
> 'true' and 'false', but again I do not think it is worth the
> confusion factor, as 'always' and 'true' already mean different
> things in some other contexts.
>
> Thanks.
>
>
>

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Is there a common utility function that does what we want? Basically
> git_config_maybe_bool but not specifically about configs.

Interesting.  git_config_maybe_bool() and its friends take the usual
(name, value) and pretend to be part of the "config" family, primarily
because that was where they came from.

But they do not really care about "name", which is used for error
reporting and that is what makes them look very specific to the
config subsystem.

I did a quick grep of git_config_maybe_bool() and I _think_ all
callers are prepared to handle errors themselves, so it might be a
good direction to go in the longer term to drop "name" and rename
the function to git_parse_maybe_bool() or something, and make these
callers use that.

In the shorter term, at least we should be able to introduce
git_parse_maybe_bool() that does not take "name", use that as a
helper to implement git_config_maybe_bool(), so that the existing
callers of git_config_maybe_bool() does not have to change.  And
that new helper can be used as your "Basically it, but not
specifically about configs".

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Dave Borowitz]

On Mon, Aug 17, 2015 at 3:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> In the shorter term, at least we should be able to introduce
> git_parse_maybe_bool() that does not take "name", use that as a
> helper to implement git_config_maybe_bool(), so that the existing
> callers of git_config_maybe_bool() does not have to change.  And
> that new helper can be used as your "Basically it, but not
> specifically about configs".

Will do, thanks for the suggestion.

Slight digression for a question that came up during reworking the
series: would it be reasonable to rewrite option parsing in
builtin/send-pack.c to use the options API? That way we can easily
reuse the option callback from builtin/push.c. (It would have some
side effects like making --no-* variants work where they did not
before; I assume that's a good thing, but it's marginally inconsistent
with some other plumbing commands like receive-pack.)

Re: [PATCH 0/7] Flags and config to sign pushes by default

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Slight digression for a question that came up during reworking the
> series: would it be reasonable to rewrite option parsing in
> builtin/send-pack.c to use the options API?

Surely.  The part of the system whose option parsing predates
parse-options may not have been converted to do so yet, but as long
as the result is correct, why not.  After all APIs were invented to
be used.

> That way we can easily
> reuse the option callback from builtin/push.c. (It would have some
> side effects like making --no-* variants work where they did not
> before; I assume that's a good thing, but it's marginally inconsistent
> with some other plumbing commands like receive-pack.)

As long as people are not deliberately feeding --no-something and
relying on it to fail, we'd be ok ;-).