On Mon, Aug 17, 2015 at 1:13 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Dave Borowitz <dborowitz@xxxxxxxxxx> 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html