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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2015, 17:13 UTC
Message-ID: <xmqqy4h9et2m.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1439492451-11233-8-git-send-email-dborowitz@google.com>

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")?

Show 8 quoted lines

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

Show 14 quoted lines

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

Show 14 quoted lines

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