Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
On 8/5/2014 12:29 AM, Eric Sunshine wrote:
On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra tanay...@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 tanay...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
Tanay Abhra tanay...@gmail.com writes:
On 8/5/2014 12:29 AM, Eric Sunshine wrote:
On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra tanay...@gmail.com wrote:
- 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
I first got convinced by Eric, but I now think you're right.
Eric's point was (I think) not about multiple entries for the same
variable, but about multiple entries for different services, like
http.receivepack = 1
http.uploadpack = 0
The order of assignments to svn-enabled change, but it doesn't matter
because svc is just a local variable pointing to the right element of
rpc_service[i]. So in both cases, you'll assign
rpc_service[index of service].enabled = last occurence of variable
http.service
even though the order of assignments will change.
Eric, am I interpreting right?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
On Wed, Aug 6, 2014 at 11:44 AM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Tanay Abhra tanay...@gmail.com writes:
On 8/5/2014 12:29 AM, Eric Sunshine wrote:
On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra tanay...@gmail.com wrote:
- 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
I first got convinced by Eric, but I now think you're right.
Eric's point was (I think) not about multiple entries for the same
variable, but about multiple entries for different services, like
http.receivepack = 1
http.uploadpack = 0
The order of assignments to svn-enabled change, but it doesn't matter
because svc is just a local variable pointing to the right element of
rpc_service[i]. So in both cases, you'll assign
rpc_service[index of service].enabled = last occurence of variable
http.service
even though the order of assignments will change.
Eric, am I interpreting right?
Yes. During initial review, I either didn't read the old code closely
enough or I misinterpreted it. Upon re-read, Tanay's rewrite looks
fine.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
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 tanay...@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);
}
- /* 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra tanay...@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 tanay...@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?
- /* 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
