| Message ID | patch-06.10-9c36f17481b-20221026T151328Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config API: make "multi" safe, fix numerous segfaults | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Refactor a "do I have an element like this?" pattern added in [1] and > [2] to use unsorted_string_list_has_string() instead of a > for_each_string_list_item() loop. In the longer term, I am not sure if we want to keep such code that uses string-list as a "database to be looked up with the string as the key". I am not sure it is worth our review bandwidth to change a for-each-string-list that terminates early to its shorthand unsorted_string_list_has_string(). Surely each such conversation would allow us to lose 4 to 5 lines, but longer term we should be discuraging the use of unsorted_string_list_has_string() in the first place.
On Thu, Oct 27 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Refactor a "do I have an element like this?" pattern added in [1] and >> [2] to use unsorted_string_list_has_string() instead of a >> for_each_string_list_item() loop. > > In the longer term, I am not sure if we want to keep such code that > uses string-list as a "database to be looked up with the string as > the key". I am not sure it is worth our review bandwidth to change > a for-each-string-list that terminates early to its shorthand > unsorted_string_list_has_string(). Surely each such conversation > would allow us to lose 4 to 5 lines, but longer term we should be > discuraging the use of unsorted_string_list_has_string() in the > first place. I haven't benchmarked, but I'd think on modern computers O(n) for such short lists would be more performance due to cache locality, i.e. not worth pre-sorting it, or making it a hash table. But for such small amounts of data I'd think it would be fine either way. As to the change, I'm fine with leaving this out. The reason it's in here is because this series came out as a reply to Stolee's earlier RFC. I think it's a fair summary to say that the reason we started talking about this at all is because the topic at hand was how to make this exact code in builtin/gc.c safer and more idiomatic. I.e. see: https://lore.kernel.org/git/e06cb4df081bc2222731f9185a22ed7ad67e3814.1664287711.git.gitgitgadget@gmail.com/ And my earlier summary of that, the very beginning showing the API forms under discussion: https://lore.kernel.org/git/220928.868rm3w9d4.gmgdl@evledraar.gmail.com/ So Re this & your "it might be good, but why does it have to be done here now?" reply to the CL: Yeah I can eject some of this, but having a series (and a predecessor RFC) whose main reason for existing is making this API safer & nicer seems incomplete unless we're also converting callers to use those supposedly nicer patterns. In general I think this sort of change is exactly the sort of thing you'd want in such a series. A test of a good API isn't just that it looks or acts nicely in isolation, but that it's easily combined with other things you might expect to use with it. Hence this 04-06/10. I.e. the original contention in the RFC was that we had to return a dummy string list to make these sort of patterns safer/nicer/idiomatic. I think these patches serve as a convincing counter-argument to that.
diff --git a/builtin/gc.c b/builtin/gc.c index 04c48638ef4..f435eda2e73 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1467,7 +1467,6 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) const char *key = "maintenance.repo"; char *config_value; char *maintpath = get_maintpath(); - struct string_list_item *item; const struct string_list *list; argc = parse_options(argc, argv, prefix, options, @@ -1485,14 +1484,8 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) else git_config_set("maintenance.strategy", "incremental"); - if (!git_config_get_knownkey_value_multi(key, &list)) { - for_each_string_list_item(item, list) { - if (!strcmp(maintpath, item->string)) { - found = 1; - break; - } - } - } + if (!git_config_get_knownkey_value_multi(key, &list)) + found = unsorted_string_list_has_string(list, maintpath); if (!found) { int rc; @@ -1532,7 +1525,6 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi const char *key = "maintenance.repo"; char *maintpath = get_maintpath(); int found = 0; - struct string_list_item *item; const struct string_list *list; argc = parse_options(argc, argv, prefix, options, @@ -1541,14 +1533,8 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi usage_with_options(builtin_maintenance_unregister_usage, options); - if (!git_config_get_knownkey_value_multi(key, &list)) { - for_each_string_list_item(item, list) { - if (!strcmp(maintpath, item->string)) { - found = 1; - break; - } - } - } + if (!git_config_get_knownkey_value_multi(key, &list)) + found = unsorted_string_list_has_string(list, maintpath); if (found) { int rc;
Refactor a "do I have an element like this?" pattern added in [1] and [2] to use unsorted_string_list_has_string() instead of a for_each_string_list_item() loop. A preceding commit added a "const" to the "struct string_list *" argument of unsorted_string_list_has_string(), it'll thus play nicely with git_config_get_const_value_multi() without needing a cast here. 1. 1ebe6b02970 (maintenance: add 'unregister --force', 2022-09-27) 2. 50a044f1e40 (gc: replace config subprocesses with API calls, 2022-09-27) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/gc.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-)