Re: [PATCH 5/3] revert most of the http_options() change
"Kyle J. McKay" writes: > And now all the t5200-url-normalize tests pass again. > > FYI, I couldn't get the patches to apply against next or pu without > some minor tweaks that were just conflict resolutions having to do > with git_config_with_options changing its signature. Thanks. I built these five patches directly on top of your series, i.e. on top of cea9928d (docs: update http..* options documentation, 2013-07-25). I'll rebuild the series with your fixes and I may even queue it to 'pu', but with some random thoughts: * "url-match.[ch]" are ugly names. I'd be happier with "urlmatch.[ch]". * In the end result, http_options() looks mostly identical to that of the 'master', but it got an extra "void *matched", only to support "git config --get-urlmatch". I however do not have to, if I do a few things: - Instead of implementing urlmatch_config_item that extends urlmatch_item, have a separate string_list, keyed by the variable names, and point the string_list with the generic cb pointer I already have in urlmatch_config. The fn() on the "git config" side has to index this second string_list with the variable and keep track of the value from the best candidate we have seen so far. - The above allows us to lose item_alloc and item_clear callback functions from urlmatch_config, as we will not be doing the structure inheritance to extend urlmatch_item. - We still need cascade_fn callback, though. * With the above, http_options() does not have to change in the series. We could restructure the series perhaps this way: - http.sslCertPasswordProtected parsing fix. - Add urlmatch.[ch], which would be your "config: improve support for http..* settings" and yesterday's "url-match: split out URL matching logic out of http.c", and a half of "url-match: generalize configuration collection logic". - Update http.c to use urlmatch.[ch] to make http.c match the result of the endgame patch [5/3], and add necessary end user documentation to Documentation/config.txt. - Add test-url-normalize and t/t5200 - Update builtin/config.c for "--get-urlmatch", to make builtin/config.c match the result of the endgame patch [5/3], add end user documentation to Documentation/git-config.txt. - Add some tests for "git config --get-urlmatch". -- 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 5/3] revert most of the http_options() change
On Jul 30, 2013, at 12:14, Kyle J. McKay wrote:
On Jul 29, 2013, at 19:13, Junio C Hamano wrote:
With the previous preparation step, the earlier 1bb6 (config:
add support for http..* settings, 2013-07-21) that introduced
many repeated changes:
-if (!strcmp("http.key", var)) {
+if (!strcmp("key", key)) {
+ if (match_is_shorter(..., OPT_KEY_NAME))
+ return 0;
... original processing for KEY_NAME ...
}
can be reverted.
This allows us to also get rid of the "repeating yourself to the
death" enum http_option_type, and new code (like db/http-savecookies
patch) that wants to add a new configuration to the http.* subsystem
does not have to conflict with http..variable series at
all.
This is more-or-less how I want the endgame to look like. Not even
compile tested, but it is getting late so I'll show it to the list
in case people may want to play with it and polish it while I am
away for the night ;-).
If people can agree that this is going in the right direction,
perhaps we should rebuild Kyle's series without detouring of adding
and then yanking what 1bb6 (config: add support for http..*
settings, 2013-07-21) did in the next cycle.
http.c | 181
+--
test-url-normalize.c | 9 ++-
2 files changed, 39 insertions(+), 151 deletions(-)
diff --git a/http.c b/http.c
index a91a00b..c7f513b 100644
--- a/http.c
+++ b/http.c
[...]
@@ -469,15 +342,23 @@ void http_init(struct remote *remote, const
char *url, int proactive_auth)
{
char *low_speed_limit;
char *low_speed_time;
- struct url_info info;
+ char *normalized_url;
+ struct urlmatch_config config = { STRING_LIST_INIT_DUP };
+
+ config.fn = http_options;
+ config.cascade_fn = git_default_config;
+ config.item_alloc = NULL;
+ config.item_clear = NULL;
+ config.cb = NULL;
Missing this:
+ config.section = "http";
[...]
diff --git a/test-url-normalize.c b/test-url-normalize.c
index 0b809eb..fab94e5 100644
--- a/test-url-normalize.c
+++ b/test-url-normalize.c
@@ -15,8 +15,15 @@ static int run_http_options(const char *file,
{
struct strbuf opt_lc;
size_t i, len;
+ struct urlmatch_config config = { STRING_LIST_INIT_DUP };
Also needs this:
+ memcpy(&config.url, info, sizeof(struct url_info));
- if (git_config_with_options(http_options, (void *)info, file, 0))
+ config.fn = http_options;
+ config.cascade_fn = git_default_config;
+ config.item_alloc = NULL;
+ config.item_clear = NULL;
+ config.cb = NULL;
Missing this:
+ config.section = "http";
Without these it segfaults running the tests.
However, now the tests that actually check the results using config
files are failing:
# failed 1 among 11 test(s)
I'm looking at it now to see if I can fix the problem.
And now all the t5200-url-normalize tests pass again.
FYI, I couldn't get the patches to apply against next or pu without
some minor tweaks that were just conflict resolutions having to do
with git_config_with_options changing its signature.
--
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 5/3] revert most of the http_options() change
On Jul 29, 2013, at 19:13, Junio C Hamano wrote:
With the previous preparation step, the earlier 1bb6 (config:
add support for http..* settings, 2013-07-21) that introduced
many repeated changes:
-if (!strcmp("http.key", var)) {
+if (!strcmp("key", key)) {
+ if (match_is_shorter(..., OPT_KEY_NAME))
+ return 0;
... original processing for KEY_NAME ...
}
can be reverted.
This allows us to also get rid of the "repeating yourself to the
death" enum http_option_type, and new code (like db/http-savecookies
patch) that wants to add a new configuration to the http.* subsystem
does not have to conflict with http..variable series at
all.
This is more-or-less how I want the endgame to look like. Not even
compile tested, but it is getting late so I'll show it to the list
in case people may want to play with it and polish it while I am
away for the night ;-).
If people can agree that this is going in the right direction,
perhaps we should rebuild Kyle's series without detouring of adding
and then yanking what 1bb6 (config: add support for http..*
settings, 2013-07-21) did in the next cycle.
http.c | 181
+--
test-url-normalize.c | 9 ++-
2 files changed, 39 insertions(+), 151 deletions(-)
diff --git a/http.c b/http.c
index a91a00b..c7f513b 100644
--- a/http.c
+++ b/http.c
[...]
@@ -469,15 +342,23 @@ void http_init(struct remote *remote, const
char *url, int proactive_auth)
{
char *low_speed_limit;
char *low_speed_time;
- struct url_info info;
+ char *normalized_url;
+ struct urlmatch_config config = { STRING_LIST_INIT_DUP };
+
+ config.fn = http_options;
+ config.cascade_fn = git_default_config;
+ config.item_alloc = NULL;
+ config.item_clear = NULL;
+ config.cb = NULL;
Missing this:
+ config.section = "http";
[...]
diff --git a/test-url-normalize.c b/test-url-normalize.c
index 0b809eb..fab94e5 100644
--- a/test-url-normalize.c
+++ b/test-url-normalize.c
@@ -15,8 +15,15 @@ static int run_http_options(const char *file,
{
struct strbuf opt_lc;
size_t i, len;
+ struct urlmatch_config config = { STRING_LIST_INIT_DUP };
- if (git_config_with_options(http_options, (void *)info, file, 0))
+ config.fn = http_options;
+ config.cascade_fn = git_default_config;
+ config.item_alloc = NULL;
+ config.item_clear = NULL;
+ config.cb = NULL;
Missing this:
+ config.section = "http";
Without these it segfaults running the tests.
However, now the tests that actually check the results using config
files are failing:
# failed 1 among 11 test(s)
I'm looking at it now to see if I can fix the problem.
--
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
