From 910d07a861d3dd1cae7ee1ae85f60184c32087e8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Mar 2023 15:16:26 -0400 Subject: [PATCH 1/4] mailmap: drop debugging code There's some debugging code in mailmap.c which is only compiled if you manually tweak the source to set DEBUG_MAILMAP. When it's not set, the fallback noop uses static inline functions; we couldn't use macros here because one of the functions is variadic (and variadic macros were forbidden back then, but aren't now). As a result, this triggers a -Wunused-parameter warning. We have a few options here: 1. Leave it be. Just mark it as UNUSED, or switch to a variadic macro. 2. Assume the debugging code is useful, compile it always, and trigger it with a run-time flag (e.g., with a trace key). This is pretty easy to do, and carries a pretty small runtime cost. 3. Assume the debugging is not very useful, and just rip it out. This matches what we did with a similar case in 69c5f17f11 (attr: drop DEBUG_ATTR code, 2022-10-06). The debugging flag has been mentioned only three times on the list. Once, when it was added in 2009: https://lore.kernel.org/git/cover.1234102794.git.marius@trolltech.com/ In 2013, when somebody fixed some compilation errors in the conditional code (presumably because they used it while making other changes): https://lore.kernel.org/git/1373871253-96480-1-git-send-email-sunshine@sunshineco.com/ And finally it seemed to have been useful to somebody in 2020: https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/ So it's not totally without value. On the other hand, it's not likely to be useful to non-developers (and certainly isn't if you have to recompile). And using a debugger or adding your own inspection code is likely to be as useful. So I've just dropped the code entirely here. Note that we do still have to mark a few parameters unused in callback functions which are passed to string_list_clear_func(). Those get an extra pointer with the string being cleared, which we previously fed to the debugging code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- mailmap.c | 42 +++--------------------------------------- 1 file changed, 3 insertions(+), 39 deletions(-) diff --git a/mailmap.c b/mailmap.c index da2589b082..22e90129ea 100644 --- a/mailmap.c +++ b/mailmap.c @@ -3,16 +3,6 @@ #include "mailmap.h" #include "object-store.h" -#define DEBUG_MAILMAP 0 -#if DEBUG_MAILMAP -#define debug_mm(...) fprintf(stderr, __VA_ARGS__) -#define debug_str(X) ((X) ? (X) : "(none)") -#else -__attribute__((format (printf, 1, 2))) -static inline void debug_mm(const char *format, ...) {} -static inline const char *debug_str(const char *s) { return s; } -#endif - const char *git_mailmap_file; const char *git_mailmap_blob; @@ -30,23 +20,17 @@ struct mailmap_entry { struct string_list namemap; }; -static void free_mailmap_info(void *p, const char *s) +static void free_mailmap_info(void *p, const char *s UNUSED) { struct mailmap_info *mi = (struct mailmap_info *)p; - debug_mm("mailmap: -- complex: '%s' -> '%s' <%s>\n", - s, debug_str(mi->name), debug_str(mi->email)); free(mi->name); free(mi->email); free(mi); } -static void free_mailmap_entry(void *p, const char *s) +static void free_mailmap_entry(void *p, const char *s UNUSED) { struct mailmap_entry *me = (struct mailmap_entry *)p; - debug_mm("mailmap: removing entries for <%s>, with %"PRIuMAX" sub-entries\n", - s, (uintmax_t)me->namemap.nr); - debug_mm("mailmap: - simple: '%s' <%s>\n", - debug_str(me->name), debug_str(me->email)); free(me->name); free(me->email); @@ -93,8 +77,6 @@ static void add_mapping(struct string_list *map, } if (!old_name) { - debug_mm("mailmap: adding (simple) entry for '%s'\n", old_email); - /* Replace current name and new email for simple entry */ if (new_name) { free(me->name); @@ -106,15 +88,10 @@ static void add_mapping(struct string_list *map, } } else { struct mailmap_info *mi = xcalloc(1, sizeof(struct mailmap_info)); - debug_mm("mailmap: adding (complex) entry for '%s'\n", old_email); mi->name = xstrdup_or_null(new_name); mi->email = xstrdup_or_null(new_email); string_list_insert(&me->namemap, old_name)->util = mi; } - - debug_mm("mailmap: '%s' <%s> -> '%s' <%s>\n", - debug_str(old_name), old_email, - debug_str(new_name), debug_str(new_email)); } static char *parse_name_and_email(char *buffer, char **name, @@ -250,11 +227,8 @@ int read_mailmap(struct string_list *map) void clear_mailmap(struct string_list *map) { - debug_mm("mailmap: clearing %"PRIuMAX" entries...\n", - (uintmax_t)map->nr); map->strdup_strings = 1; string_list_clear_func(map, free_mailmap_entry); - debug_mm("mailmap: cleared\n"); } /* @@ -315,10 +289,6 @@ int map_user(struct string_list *map, struct string_list_item *item; struct mailmap_entry *me; - debug_mm("map_user: map '%.*s' <%.*s>\n", - (int)*namelen, debug_str(*name), - (int)*emaillen, debug_str(*email)); - item = lookup_prefix(map, *email, *emaillen); if (item) { me = (struct mailmap_entry *)item->util; @@ -336,10 +306,8 @@ int map_user(struct string_list *map, } if (item) { struct mailmap_info *mi = (struct mailmap_info *)item->util; - if (mi->name == NULL && mi->email == NULL) { - debug_mm("map_user: -- (no simple mapping)\n"); + if (mi->name == NULL && mi->email == NULL) return 0; - } if (mi->email) { *email = mi->email; *emaillen = strlen(*email); @@ -348,11 +316,7 @@ int map_user(struct string_list *map, *name = mi->name; *namelen = strlen(*name); } - debug_mm("map_user: to '%.*s' <%.*s>\n", - (int)*namelen, debug_str(*name), - (int)*emaillen, debug_str(*email)); return 1; } - debug_mm("map_user: --\n"); return 0; } From 647edf79d6b61391392ed42435b3214c02f539f1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Mar 2023 15:16:34 -0400 Subject: [PATCH 2/4] http: drop unused parameter from start_object_request() We take a "walker" parameter for the request, but don't actually look at it. This is due to 5424bc557f (http*: add helper methods for fetching objects (loose), 2009-06-06). Before then, we consulted the "walker" struct to tell us if we should be verbose, but now those messages are printed elsewhere. Let's drop the unused parameter to make -Wunused-parameter happy. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-walker.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/http-walker.c b/http-walker.c index c3e902c40e..065a03d262 100644 --- a/http-walker.c +++ b/http-walker.c @@ -53,8 +53,7 @@ static void fetch_alternates(struct walker *walker, const char *base); static void process_object_response(void *callback_data); -static void start_object_request(struct walker *walker, - struct object_request *obj_req) +static void start_object_request(struct object_request *obj_req) { struct active_request_slot *slot; struct http_object_request *req; @@ -111,7 +110,7 @@ static void process_object_response(void *callback_data) obj_req->repo = obj_req->repo->next; release_http_object_request(obj_req->req); - start_object_request(walker, obj_req); + start_object_request(obj_req); return; } } @@ -139,7 +138,7 @@ static int fill_active_slot(struct walker *walker) if (has_object_file(&obj_req->oid)) obj_req->state = COMPLETE; else { - start_object_request(walker, obj_req); + start_object_request(obj_req); return 1; } } From 1e5e0974964170a917d6950dda74e82745ab65aa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Mar 2023 15:17:08 -0400 Subject: [PATCH 3/4] http: mark unused parameter in fill_active_slot() callbacks We have a generic "fill" function that is used by both the dumb http push and fetch code paths. It takes a void parameter in case the caller wants to pass along extra data, but (since the previous commit) neither does so. So we could simply drop the extra parameter. But since it's good practice to provide a void pointer for in callback functions, we'll leave it here for the future, and just annotate it as unused (to appease -Wunused-parameter). While we're marking it, let's also fix the type in http-walker's function to have the correct "void" type. The original had to cast the function pointer and was technically undefined behavior (though generally OK in practice). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 2 +- http-walker.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/http-push.c b/http-push.c index 88aa045ecb..2d01b430e7 100644 --- a/http-push.c +++ b/http-push.c @@ -602,7 +602,7 @@ static void finish_request(struct transfer_request *request) } static int is_running_queue; -static int fill_active_slot(void *unused) +static int fill_active_slot(void *data UNUSED) { struct transfer_request *request; diff --git a/http-walker.c b/http-walker.c index 065a03d262..93a4a98a3d 100644 --- a/http-walker.c +++ b/http-walker.c @@ -127,7 +127,7 @@ static void release_object_request(struct object_request *obj_req) free(obj_req); } -static int fill_active_slot(struct walker *walker) +static int fill_active_slot(void *data UNUSED) { struct object_request *obj_req; struct list_head *pos, *tmp, *head = &object_queue_head; @@ -613,7 +613,7 @@ struct walker *get_http_walker(const char *url) walker->cleanup = cleanup; walker->data = data; - add_fill_function(walker, (int (*)(void *)) fill_active_slot); + add_fill_function(NULL, fill_active_slot); return walker; } From b3edf335dfa978b43d10de7e722a7e355a3d0e4c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Mar 2023 15:17:18 -0400 Subject: [PATCH 4/4] transport: mark unused parameters in fetch_refs_from_bundle() We don't look at the "to_fetch" or "nr_heads" parameters at all. At first glance this seems like a bug (or at least pessimisation), because it means we fetch more objects from the bundle than we actually need. But the bundle does not have any way of computing the set of reachable objects itself (we'd have to pull all of the objects out to walk them). And anyway, we've probably already paid most of the cost of grabbing the objects, since we must copy the bundle locally before accessing it. So it's perfectly reasonable for the bundle code to just pull everything into the local object store. Unneeded objects can be dropped later via gc, etc. But we should mark these unused parameters as such to avoid the wrath of -Wunused-parameter. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- transport.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/transport.c b/transport.c index 906dbad5a0..bb532d0f72 100644 --- a/transport.c +++ b/transport.c @@ -170,7 +170,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport, } static int fetch_refs_from_bundle(struct transport *transport, - int nr_heads, struct ref **to_fetch) + int nr_heads UNUSED, + struct ref **to_fetch UNUSED) { struct bundle_transport_data *data = transport->data; struct strvec extra_index_pack_args = STRVEC_INIT;