[PATCH 0/4] more unused-parameter fixes

[PATCH 0/4] more unused-parameter fixes

[Jeff King]

Here are a few more patches to silence some -Wunused-parameter warnings.
I've pulled these ones out specifically because they're a little more
interesting than just "slap an UNUSED marker on the variable".

  [1/4]: mailmap: drop debugging code
  [2/4]: http: drop unused parameter from start_object_request()
  [3/4]: http: mark unused parameter in fill_active_slot() callbacks
  [4/4]: transport: mark unused parameters in fetch_refs_from_bundle()

 http-push.c   |  2 +-
 http-walker.c | 11 +++++------
 mailmap.c     | 42 +++---------------------------------------
 transport.c   |  3 ++-
 4 files changed, 11 insertions(+), 47 deletions(-)

-Peff

[PATCH 1/4] mailmap: drop debugging code

[Jeff King]

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 2021:

  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 <peff@peff.net>
---
I'm cc-ing folks from those earlier threads. If somebody really wants to
salvage it, I can prepare a patch converting it to a trace variable
instead, but absent any outcry, I'd go with this approach.

 mailmap.c | 42 +++---------------------------------------
 1 file changed, 3 insertions(+), 39 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index da2589b0822..22e90129ea8 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;
 }
-- 
2.40.0.581.g8d688c70eca

Re: [PATCH 1/4] mailmap: drop debugging code

[Eric Sunshine]

On Fri, Mar 17, 2023 at 3:16 PM Jeff King <peff@peff.net> wrote:
> 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 2021:
>
>   https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/

Nit: s/2021/2020/

> 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 <peff@peff.net>
> ---
> I'm cc-ing folks from those earlier threads. If somebody really wants to
> salvage it, I can prepare a patch converting it to a trace variable
> instead, but absent any outcry, I'd go with this approach.

As one of the mentioned anonymous "sombody"s, I have no objection.

Re: [PATCH 1/4] mailmap: drop debugging code

[Jeff King]

On Fri, Mar 17, 2023 at 04:08:43PM -0400, Eric Sunshine wrote:

> > 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 2021:
> >
> >   https://lore.kernel.org/git/87eejswql6.fsf@evledraar.gmail.com/
> 
> Nit: s/2021/2020/

Oops. I'm not sure how I bungled that. :)

> > I'm cc-ing folks from those earlier threads. If somebody really wants to
> > salvage it, I can prepare a patch converting it to a trace variable
> > instead, but absent any outcry, I'd go with this approach.
> 
> As one of the mentioned anonymous "sombody"s, I have no objection.

Thanks!

-Peff

[PATCH 2/4] http: drop unused parameter from start_object_request()

[Jeff King]

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 <peff@peff.net>
---
 http-walker.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index b8f0f98ae14..6b9bdb529b5 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -52,8 +52,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;
@@ -110,7 +109,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;
 		}
 	}
@@ -138,7 +137,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;
 			}
 		}
-- 
2.40.0.581.g8d688c70eca

[PATCH 3/4] http: mark unused parameter in fill_active_slot() callbacks

[Jeff King]

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 <peff@peff.net>
---
Every time I touch these files I dream of just deleting all of the
dumb-http completely.

 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 7f71316456c..8eaba6ddf0a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -601,7 +601,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 6b9bdb529b5..32369aa2d74 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -126,7 +126,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;
@@ -612,7 +612,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;
 }
-- 
2.40.0.581.g8d688c70eca

[PATCH 4/4] transport: mark unused parameters in fetch_refs_from_bundle()

[Jeff King]

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 <peff@peff.net>
---
 transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/transport.c b/transport.c
index 81950bf755f..c5abf26f31e 100644
--- a/transport.c
+++ b/transport.c
@@ -167,7 +167,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;
-- 
2.40.0.581.g8d688c70eca

Re: [PATCH 4/4] transport: mark unused parameters in fetch_refs_from_bundle()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Very well described, and I agree with the reasoning.

Will queue all four.  Thanks.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  transport.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/transport.c b/transport.c
> index 81950bf755f..c5abf26f31e 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -167,7 +167,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;