Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family

[Tanay Abhra <tanayabh@gmail.com>]

On 8/5/2014 12:29 AM, Eric Sunshine wrote:
> On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use `git_config_get_bool()` family instead of `git_config()` to take advantage of
>> the config-set API which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  http-backend.c | 31 ++++++++++++-------------------
>>  1 file changed, 12 insertions(+), 19 deletions(-)
>>
>> diff --git a/http-backend.c b/http-backend.c
>> index 80790bb..106ca6b 100644
>> --- a/http-backend.c
>> +++ b/http-backend.c
>> @@ -219,29 +219,22 @@ static void get_idx_file(char *name)
>>         send_local_file("application/x-git-packed-objects-toc", name);
>>  }
>>
>> -static int http_config(const char *var, const char *value, void *cb)
>> +static void http_config(void)
>>  {
>> -       const char *p;
>> +       int i, value = 0;
>> +       struct strbuf var = STRBUF_INIT;
>>
>> -       if (!strcmp(var, "http.getanyfile")) {
>> -               getanyfile = git_config_bool(var, value);
>> -               return 0;
>> -       }
>> +       git_config_get_bool("http.getanyfile", &getanyfile);
>>
>> -       if (skip_prefix(var, "http.", &p)) {
>> -               int i;
>> -
>> -               for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>> -                       struct rpc_service *svc = &rpc_service[i];
>> -                       if (!strcmp(p, svc->config_name)) {
>> -                               svc->enabled = git_config_bool(var, value);
>> -                               return 0;
>> -                       }
>> -               }
>> +       for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
>> +               struct rpc_service *svc = &rpc_service[i];
>> +               strbuf_addf(&var, "http.%s", svc->config_name);
>> +               if (!git_config_get_bool(var.buf, &value))
>> +                       svc->enabled = value;
>> +               strbuf_reset(&var);
>>         }
> 
> There is a behavior change here. The original code set svc->enabled to
> the first matching rpc_service[] entry, whereas the new code sets it
> to the last matching entry. Is this change intentional?
>

I was preparing the reroll and I saw that I had missed your mail.
I think that I haven't changed the behaviour, the original one is
written in callback form so it has to go through the array every time for each
new value.
When there are multiple entries for a service say,

http.receivepack = 1
http.receivepack = 0

the old code would have at overwritten the previous entry with the new value.
The new code just populates the whole array of `rpc_service` in one go, choosing
the last matching value for each entry. For reviewing purpose the original array is
this,

	struct rpc_service {
	const char *name;
	const char *config_name;
	signed enabled : 2;
	};

	static struct rpc_service rpc_service[] = {
		{ "upload-pack", "uploadpack", 1 },
		{ "receive-pack", "receivepack", -1 },
	};

What do you think, am I interpreting it wrong? Thanks.

>> -       /* we are not interested in parsing any other configuration here */
>> -       return 0;
>> +       strbuf_release(&var);
>>  }
>>
>>  static struct rpc_service *select_service(const char *name)
>> @@ -627,7 +620,7 @@ int main(int argc, char **argv)
>>             access("git-daemon-export-ok", F_OK) )
>>                 not_found("Repository not exported: '%s'", dir);
>>
>> -       git_config(http_config, NULL);
>> +       http_config();
>>         cmd->imp(cmd_arg);
>>         return 0;
>>  }
>> --
>> 1.9.0.GIT