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

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