On Mon, Feb 2, 2009 at 3:31 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Applying style fixes to the existing code is very much appreciated, *but*
> please make such a clean-up patch a separate one. A two-patch series
> whose [1/2] is such a pure clean-up without any feature change, with [2/2]
> that adds code to the cleaned-up state would be much less distracting for
> people who nitpick your changes.
Okay, I'll try to do so next time.
>> @@ -153,11 +159,69 @@ static int http_options(const char *var, const char *value, void *cb)
>> return git_config_string(&curl_http_proxy, var, value);
>> return 0;
>> }
>> +#if LIBCURL_VERSION_NUM >= 0x070a06
>> + if (!strcmp("http.auth", var)) {
>> + if (curl_http_auth == NULL)
>> + return git_config_string(&curl_http_auth, var, value);
>> + return 0;
>> + }
>> +#endif
>> +#if LIBCURL_VERSION_NUM >= 0x070a07
>> + if (!strcmp("http.proxy_auth", var)) {
>> + if (curl_http_proxy_auth == NULL)
>> + return git_config_string(&curl_http_proxy_auth, var, value);
>> + return 0;
>> + }
>> +#endif
>
> If you follow config.c::git_config() you will notice that we read from
> /etc/gitconfig, $HOME/.gitconfig and then finally $GIT_DIR/config. By
> implementing "if we already have read curl_http_auth already, we will
> ignore the later setting" like above code does, you break the general
> expectation that system-wide defaults is overridable by $HOME/.gitconfig
> and that is in turn overridable by per-repository $GIT_DIR/config.
But aren't the globals supposed to be set just here? I guessed you
assume that these are set elsewhere and then it prevents the values
provided later from being applied.
> The preferred order would be:
>
> - Use the value obtained from command line parameters, if any;
>
> - Otherwise, if an environment variable is there, use it;
>
> - Otherwise, the value obtained from git_config(), with "later one wins"
> rule.
>
> I think you are not adding any command line option, so favoring
> environment and then using configuration is fine, but the configuration
> parser must follow the usual "later one wins" rule to avoid dissapointing
> the users.
I just followed the way other options behave. I was just not sure how
I was supposed to deal with them.
>> +#if LIBCURL_VERSION_NUM >= 0x070a06
>> +static long get_curl_auth_bitmask(const char* auth_method)
>
> In git codebase, asterisk that means "a pointer" sticks to the variable
> name not to type name; "const char *auth_method" (I see this file is
> already infested with such style violations, but if you are doing a
> separate clean-up patch it would be appreciated to clean them up).
>
I'm not willing to do it this time ;-)
>> +{
>> + char buf[4096];
>
> Do you need that much space?
I think as long as we use fixed-size buffers, I should get them enough
sized. If this is not preferrable, then it'd be better off using
heap-allocated buffers.
>> + const unsigned char *p = (const unsigned char *)auth_method;
>> + long mask = CURLAUTH_NONE;
>> +
>> + strlcpy(buf, auth_method, sizeof(buf));
>
> A tab is 8-char wide.
Sorry about this. I actually was careful but I just forgot to turn off
the tab expansion for the second time.
> What happens when auth_method is longer than 4kB?
>
>> +
>> + for (;;) {
>> + char *q = buf;
>> + while (*p && isspace(*p))
>> + ++p;
>
> If there is no particular reason to choose one over the other, please use
> postincrement, p++, as other existing parts of the codebase.
>
> I'll try to demonstrate what (I think) this patch should look like as a
> pair of follow-up messages to this one, but I am not sure about my rewrite
> of get_curl_auth_bitmask(). Please consider it as an easter-egg bughunt
> ;-)
I anyway appreciate this kind of knit-picking as I'd do so to newbies.
Thanks very much for the advice.
Regards,
Moriyoshi
--
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
