[PATCH 0/22] YAUPS: Yet Another Unused Parameter Series

[PATCH 0/22] YAUPS: Yet Another Unused Parameter Series

[Jeff King]

I'm back with another exciting installment of -Wunused-parameter warning
fixes. Most of these are pretty boring and obvious; the first two are
the most interesting in terms of rationale.

I promise we're closing in on the finish line here. I only have about 20
patches left after this, at which point we should be able to turn on the
warning by default for developer builds.

  [01/22]: sequencer: use repository parameter in short_commit_name()
  [02/22]: sequencer: mark repository argument as unused
  [03/22]: ref-filter: mark unused parameters in parser callbacks
  [04/22]: pack-bitmap: mark unused parameters in show_object callback
  [05/22]: worktree: mark unused parameters in each_ref_fn callback
  [06/22]: commit-graph: mark unused data parameters in generation callbacks
  [07/22]: ls-tree: mark unused parameter in callback
  [08/22]: stash: mark unused parameter in diff callback
  [09/22]: trace2: mark unused us_elapsed_absolute parameters
  [10/22]: trace2: mark unused config callback parameter
  [11/22]: test-trace2: mark unused argv/argc parameters
  [12/22]: grep: mark unused parameter in output function
  [13/22]: add-interactive: mark unused callback parameters
  [14/22]: negotiator/noop: mark unused callback parameters
  [15/22]: worktree: mark unused parameters in noop repair callback
  [16/22]: imap-send: mark unused parameters with NO_OPENSSL
  [17/22]: grep: mark unused parmaeters in pcre fallbacks
  [18/22]: credential: mark unused parameter in urlmatch callback
  [19/22]: fetch: mark unused parameter in ref_transaction callback
  [20/22]: bundle-uri: mark unused parameters in callbacks
  [21/22]: gc: mark unused descriptors in scheduler callbacks
  [22/22]: update-ref: mark unused parameter in parser callbacks

 add-interactive.c       |  8 ++++----
 builtin/fetch.c         |  2 +-
 builtin/gc.c            |  6 +++---
 builtin/ls-tree.c       |  3 ++-
 builtin/stash.c         |  2 +-
 builtin/update-ref.c    | 14 +++++++-------
 builtin/worktree.c      |  8 ++++----
 bundle-uri.c            |  6 +++---
 commit-graph.c          |  8 +++++---
 credential.c            |  4 ++--
 grep.c                  | 12 +++++++-----
 imap-send.c             | 10 ++++++++--
 negotiator/noop.c       | 12 +++++++-----
 pack-bitmap.c           |  5 +++--
 ref-filter.c            |  8 +++++---
 sequencer.c             | 27 ++++++++++++++-------------
 t/helper/test-trace2.c  |  6 +++---
 trace2/tr2_sysenv.c     |  3 ++-
 trace2/tr2_tgt_event.c  | 23 +++++++++++++----------
 trace2/tr2_tgt_normal.c | 20 ++++++++++++--------
 worktree.c              |  6 ++++--
 21 files changed, 110 insertions(+), 83 deletions(-)

-Peff

[PATCH 01/22] sequencer: use repository parameter in short_commit_name()

[Jeff King]

Instead of just using the_repository, we can take a repository parameter
from the caller. Most of them already have one, and doing so clears up a
few -Wunused-parameter warnings. There are still a few callers which use
the_repository, but this pushes us one small step forward to eventually
getting rid of those.

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 48475d1cc6..82dc3e160e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -433,10 +433,9 @@ struct commit_message {
 	const char *message;
 };
 
-static const char *short_commit_name(struct commit *commit)
+static const char *short_commit_name(struct repository *r, struct commit *commit)
 {
-	return repo_find_unique_abbrev(the_repository, &commit->object.oid,
-				       DEFAULT_ABBREV);
+	return repo_find_unique_abbrev(r, &commit->object.oid, DEFAULT_ABBREV);
 }
 
 static int get_message(struct commit *commit, struct commit_message *out)
@@ -446,7 +445,7 @@ static int get_message(struct commit *commit, struct commit_message *out)
 
 	out->message = repo_logmsg_reencode(the_repository, commit, NULL,
 					    get_commit_output_encoding());
-	abbrev = short_commit_name(commit);
+	abbrev = short_commit_name(the_repository, commit);
 
 	subject_len = find_commit_subject(out->message, &subject);
 
@@ -2383,7 +2382,7 @@ static int do_pick_commit(struct repository *r,
 		error(command == TODO_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
-		      short_commit_name(commit), msg.subject);
+		      short_commit_name(r, commit), msg.subject);
 		print_advice(r, res == 1, opts);
 		repo_rerere(r, opts->allow_rerere_auto);
 		goto leave;
@@ -3172,7 +3171,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 		item->offset_in_buf = todo_list->buf.len;
 		subject_len = find_commit_subject(commit_buffer, &subject);
 		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
-			short_commit_name(commit), subject_len, subject);
+			short_commit_name(opts->revs->repo, commit),
+			subject_len, subject);
 		repo_unuse_commit_buffer(the_repository, commit,
 					 commit_buffer);
 	}
@@ -3593,7 +3593,7 @@ static int error_with_patch(struct repository *r,
 	} else if (exit_code) {
 		if (commit)
 			fprintf_ln(stderr, _("Could not apply %s... %.*s"),
-				   short_commit_name(commit), subject_len, subject);
+				   short_commit_name(r, commit), subject_len, subject);
 		else
 			/*
 			 * We don't have the hash of the parent so
@@ -4728,7 +4728,7 @@ static int pick_commits(struct repository *r,
 						term_clear_line();
 					fprintf(stderr,
 						_("Stopped at %s...  %.*s\n"),
-						short_commit_name(commit),
+						short_commit_name(r, commit),
 						item->arg_len, arg);
 				}
 				return error_with_patch(r, commit,
@@ -5564,7 +5564,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		if (!is_empty && (commit->object.flags & PATCHSAME)) {
 			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
 				warning(_("skipped previously applied commit %s"),
-					short_commit_name(commit));
+					short_commit_name(revs->repo, commit));
 			skipped_commit = 1;
 			continue;
 		}
@@ -5800,7 +5800,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 		if (!is_empty && (commit->object.flags & PATCHSAME)) {
 			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
 				warning(_("skipped previously applied commit %s"),
-					short_commit_name(commit));
+					short_commit_name(r, commit));
 			skipped_commit = 1;
 			continue;
 		}
@@ -5892,7 +5892,8 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,
 	todo_list->alloc = alloc;
 }
 
-static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_list,
+static void todo_list_to_strbuf(struct repository *r,
+				struct todo_list *todo_list,
 				struct strbuf *buf, int num, unsigned flags)
 {
 	struct todo_item *item;
@@ -5921,7 +5922,7 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 		/* add commit id */
 		if (item->commit) {
 			const char *oid = flags & TODO_LIST_SHORTEN_IDS ?
-					  short_commit_name(item->commit) :
+					  short_commit_name(r, item->commit) :
 					  oid_to_hex(&item->commit->object.oid);
 
 			if (item->command == TODO_FIXUP) {
-- 
2.42.0.505.g4c6fb48dec

Re: [PATCH 01/22] sequencer: use repository parameter in short_commit_name()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> -static const char *short_commit_name(struct commit *commit)
> +static const char *short_commit_name(struct repository *r, struct commit *commit)
>  {
> -	return repo_find_unique_abbrev(the_repository, &commit->object.oid,
> -				       DEFAULT_ABBREV);
> +	return repo_find_unique_abbrev(r, &commit->object.oid, DEFAULT_ABBREV);
>  }

Theoretically this is the right thing to do, and ...

>  static int get_message(struct commit *commit, struct commit_message *out)
> @@ -446,7 +445,7 @@ static int get_message(struct commit *commit, struct commit_message *out)
>  
>  	out->message = repo_logmsg_reencode(the_repository, commit, NULL,
>  					    get_commit_output_encoding());
> -	abbrev = short_commit_name(commit);
> +	abbrev = short_commit_name(the_repository, commit);

... this is a no-op.

> @@ -2383,7 +2382,7 @@ static int do_pick_commit(struct repository *r,
>  		error(command == TODO_REVERT
>  		      ? _("could not revert %s... %s")
>  		      : _("could not apply %s... %s"),
> -		      short_commit_name(commit), msg.subject);
> +		      short_commit_name(r, commit), msg.subject);

And this is a logical consequence for a function that is told by the
caller in which repository to work in via its parameter.

> @@ -3172,7 +3171,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
>  		item->offset_in_buf = todo_list->buf.len;
>  		subject_len = find_commit_subject(commit_buffer, &subject);
>  		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
> -			short_commit_name(commit), subject_len, subject);
> +			short_commit_name(opts->revs->repo, commit),
> +			subject_len, subject);
>  		repo_unuse_commit_buffer(the_repository, commit,
>  					 commit_buffer);

But how do we ascertain that opts->revs->repo is always safe to use
(iow initialized to a sensible value)?  repo_init_revisions() takes
a repository as its parameter and the first thing it does is to set
it to the revs->repo, so it is safe for any "struct rev_info" that
came from there, but REV_INFO_INIT omits initializer for the .repo
member.

The other two calls in this loop refer to the_repository so the
current code would be safe even if opts->revs->repo is set or NULL,
but that will no longer be true with the updated code.  I'd feel
safer if at the beginning of the function we create a local variable
"struct rev_info *repo" that is initialized to opts->revs->repo and
use it throughout the function instead of the_repository.


> @@ -5564,7 +5564,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  		if (!is_empty && (commit->object.flags & PATCHSAME)) {
>  			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
>  				warning(_("skipped previously applied commit %s"),
> -					short_commit_name(commit));
> +					short_commit_name(revs->repo, commit));
>  			skipped_commit = 1;
>  			continue;
>  		}

This one I am fairly certain is a safe and correct conversion; the
function would be segfaulting in its call to get_revision() if
revs->repo were set to a bogus value.

Thanks.

Re: [PATCH 01/22] sequencer: use repository parameter in short_commit_name()

[Taylor Blau]

On Mon, Aug 28, 2023 at 03:21:11PM -0700, Junio C Hamano wrote:
> > @@ -3172,7 +3171,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
> >  		item->offset_in_buf = todo_list->buf.len;
> >  		subject_len = find_commit_subject(commit_buffer, &subject);
> >  		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
> > -			short_commit_name(commit), subject_len, subject);
> > +			short_commit_name(opts->revs->repo, commit),
> > +			subject_len, subject);
> >  		repo_unuse_commit_buffer(the_repository, commit,
> >  					 commit_buffer);
>
> But how do we ascertain that opts->revs->repo is always safe to use
> (iow initialized to a sensible value)?  repo_init_revisions() takes
> a repository as its parameter and the first thing it does is to set
> it to the revs->repo, so it is safe for any "struct rev_info" that
> came from there, but REV_INFO_INIT omits initializer for the .repo
> member.
>
> The other two calls in this loop refer to the_repository so the
> current code would be safe even if opts->revs->repo is set or NULL,
> but that will no longer be true with the updated code.  I'd feel
> safer if at the beginning of the function we create a local variable
> "struct rev_info *repo" that is initialized to opts->revs->repo and
> use it throughout the function instead of the_repository.

This call comes from sequencer_pick_revisions() ->
walk_revs_populate_todo(), where opts is passed in from the caller of
the former function.

The sole caller of that function is run_sequencer() in builtin/revert.c.
The relevant portion of that function is:

    if (cmd) {
        opts->revs = NULL;
    } else {
        struct setup_revision_opt s_r_opt;
        opts->revs = xmalloc(sizeof(*opts->revs));
        repo_init_revisions(the_repository, opts->revs, NULL);
        opts->revs->no_walk = 1;
        opts->revs->unsorted_input = 1;
        /* ... */
    }

So as long as we end up in the else arm of this conditional, we'll have
called repo_init_revisions(), causing opts->revs->repo to be equal to
the_repository.

Thankfully, we have an assert(opts->revs) at the beginning of
sequencer_pick_revisions(), so we know that we always take the else arm
on this particular call path.

...but, sequencer_pick_revisions() itself takes a repository pointer, so
we could equally use that, or drop it, like so:

--- 8< ---
diff --git a/builtin/revert.c b/builtin/revert.c
index e6f9a1ad26..29e215c72a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -224,7 +224,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
 		return sequencer_rollback(the_repository, opts);
 	if (cmd == 's')
 		return sequencer_skip(the_repository, opts);
-	return sequencer_pick_revisions(the_repository, opts);
+	return sequencer_pick_revisions(opts);
 }

 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 48475d1cc6..bc7e687623 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5201,14 +5201,15 @@ static int single_pick(struct repository *r,
 	return do_pick_commit(r, &item, opts, 0, &check_todo);
 }

-int sequencer_pick_revisions(struct repository *r,
-			     struct replay_opts *opts)
+int sequencer_pick_revisions(struct replay_opts *opts)
 {
 	struct todo_list todo_list = TODO_LIST_INIT;
 	struct object_id oid;
+	struct repository *r;
 	int i, res;

 	assert(opts->revs);
+	r = opts->revs->repo;
 	if (read_and_refresh_cache(r, opts))
 		return -1;

diff --git a/sequencer.h b/sequencer.h
index 913a0f652d..10caa3dc93 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,8 +158,7 @@ void todo_list_filter_update_refs(struct repository *r,

 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
-int sequencer_pick_revisions(struct repository *repo,
-			     struct replay_opts *opts);
+int sequencer_pick_revisions(struct replay_opts *opts);
 int sequencer_continue(struct repository *repo, struct replay_opts *opts);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
--- >8 ---

though it makes for an awkward API to have all of the other
sequencer-related functions take as their first argument a pointer to a
repository struct, leaving sequencer_pick_revisions() as the odd one
out.

So I'd probably just as soon drop that and do what Junio suggests:

--- 8< ---
diff --git a/sequencer.c b/sequencer.c
index 82dc3e160e..6c06b8e1bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3143,22 +3143,24 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 }

 static int walk_revs_populate_todo(struct todo_list *todo_list,
-				struct replay_opts *opts)
+				   struct replay_opts *opts)
 {
 	enum todo_command command = opts->action == REPLAY_PICK ?
 		TODO_PICK : TODO_REVERT;
 	const char *command_string = todo_command_info[command].str;
 	const char *encoding;
 	struct commit *commit;
+	struct repository *r;

 	if (prepare_revs(opts))
 		return -1;
+	r = opts->revs->repo;

 	encoding = get_log_output_encoding();

 	while ((commit = get_revision(opts->revs))) {
 		struct todo_item *item = append_new_todo(todo_list);
-		const char *commit_buffer = repo_logmsg_reencode(the_repository,
+		const char *commit_buffer = repo_logmsg_reencode(r,
 								 commit, NULL,
 								 encoding);
 		const char *subject;
@@ -3173,8 +3175,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
 			short_commit_name(opts->revs->repo, commit),
 			subject_len, subject);
-		repo_unuse_commit_buffer(the_repository, commit,
-					 commit_buffer);
+		repo_unuse_commit_buffer(r, commit, commit_buffer);
 	}

 	if (!todo_list->nr)
--- >8 ---

> > @@ -5564,7 +5564,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >  		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> >  			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
> >  				warning(_("skipped previously applied commit %s"),
> > -					short_commit_name(commit));
> > +					short_commit_name(revs->repo, commit));
> >  			skipped_commit = 1;
> >  			continue;
> >  		}
>
> This one I am fairly certain is a safe and correct conversion; the
> function would be segfaulting in its call to get_revision() if
> revs->repo were set to a bogus value.

Agreed.

Thanks,
Taylor

Re: [PATCH 01/22] sequencer: use repository parameter in short_commit_name()

[Jeff King]

On Mon, Aug 28, 2023 at 03:21:11PM -0700, Junio C Hamano wrote:

> > @@ -3172,7 +3171,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
> >  		item->offset_in_buf = todo_list->buf.len;
> >  		subject_len = find_commit_subject(commit_buffer, &subject);
> >  		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
> > -			short_commit_name(commit), subject_len, subject);
> > +			short_commit_name(opts->revs->repo, commit),
> > +			subject_len, subject);
> >  		repo_unuse_commit_buffer(the_repository, commit,
> >  					 commit_buffer);
> 
> But how do we ascertain that opts->revs->repo is always safe to use
> (iow initialized to a sensible value)?  repo_init_revisions() takes
> a repository as its parameter and the first thing it does is to set
> it to the revs->repo, so it is safe for any "struct rev_info" that
> came from there, but REV_INFO_INIT omits initializer for the .repo
> member.

I mostly just assumed it would have been initialized, because passing in
anything else and expecting it to work is a bit crazy. REV_INFO_INIT
specifically says:

   * This will not fully initialize a "struct rev_info", the
   * repo_init_revisions() function needs to be called before
   * setup_revisions() and any revision walking takes place.

and then goes on to explain that the point is just so you can "goto
cleanup" and free it if you never made it to the init_revisions() call.
So the code would already be pretty buggy in this case.

That said, all of the code paths here do call repo_init_revisions(), so
I think it is OK. But if we want to make the patch simple and more
obviously correct, I would prefer to just blindly use the_repository in
callers that don't have a "struct repository" themselves. Then we know
it's a strict step forward (if slightly smaller), and we can leave the
refactoring of the rest of the sequencer code for another day. I was
trying hard to draw a reasonable line and not get this already-large
series derailed by only semi-related refactoring.

> The other two calls in this loop refer to the_repository so the
> current code would be safe even if opts->revs->repo is set or NULL,
> but that will no longer be true with the updated code.  I'd feel
> safer if at the beginning of the function we create a local variable
> "struct rev_info *repo" that is initialized to opts->revs->repo and
> use it throughout the function instead of the_repository.

I'm not sure how that makes it any safer, as it would mean using the
suspect repo pointer in more places. Unless you are proposing to do:

  struct repository *r = opts->revs->repo;
  if (!r)
	r = the_repository; /* or BUG() ? */

-Peff

Re: [PATCH 01/22] sequencer: use repository parameter in short_commit_name()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> but that will no longer be true with the updated code.  I'd feel
>> safer if at the beginning of the function we create a local variable
>> "struct rev_info *repo" that is initialized to opts->revs->repo and
>> use it throughout the function instead of the_repository.
>
> I'm not sure how that makes it any safer, as it would mean using the
> suspect repo pointer in more places. Unless you are proposing to do:
>
>   struct repository *r = opts->revs->repo;
>   if (!r)
> 	r = the_repository; /* or BUG() ? */

Yup, the BUG() variant was exactly what I had in mind, but without
the assert, by using opts->revs->repo more consistently, we would
make sure nobody will call us with uninitialized opts->revs->repo
now or in the future.

[PATCH 02/22] sequencer: mark repository argument as unused

[Jeff King]

In sequencer_get_last_command(), we don't ever look at the repository
parameter. It _should_ be used when calling into git_path_* functions,
but the one we use here is declared with the non-REPO variant of
GIT_PATH_FUNC(), and so just uses the_repository internally.

We could change the path helper to use REPO_GIT_PATH_FUNC(), but doing
so piecemeal is not great. There are 41 uses of GIT_PATH_FUNC() in
sequencer.c, and inconsistently switching one makes the code more
confusing. Likewise, this one function is used in half a dozen other
spots, all of which would need to start passing in a repository argument
(with rippling effects up the call stack).

So let's punt on that for now and just silence any -Wunused-parameter
warning.

Note that we could also drop this parameter entirely, as the function is
always called directly, and not as a callback that has to conform to
some external interface. But since we'd eventually want to use the
repository parameter, let's leave it in place to avoid disrupting the
callers twice.

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 82dc3e160e..41fd79d215 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2649,7 +2649,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 	return item->commit ? 0 : -1;
 }
 
-int sequencer_get_last_command(struct repository *r, enum replay_action *action)
+int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
 {
 	const char *todo_file, *bol;
 	struct strbuf buf = STRBUF_INIT;
-- 
2.42.0.505.g4c6fb48dec

Re: [PATCH 02/22] sequencer: mark repository argument as unused

[Taylor Blau]

On Mon, Aug 28, 2023 at 05:47:17PM -0400, Jeff King wrote:
> Note that we could also drop this parameter entirely, as the function is
> always called directly, and not as a callback that has to conform to
> some external interface. But since we'd eventually want to use the
> repository parameter, let's leave it in place to avoid disrupting the
> callers twice.

Yeah, I think that this (and the discussion on the previous patch) are
hinting that we should consider dropping the repository argument more
broadly throughout the sequencer API, since we can often use
`opts->revs->repo` in its place.

But we can definitely leave that for another day ;-). In the meantime,
this patch LGTM.

Thanks,
Taylor

Re: [PATCH 02/22] sequencer: mark repository argument as unused

[Phillip Wood]

Hi Peff

On 28/08/2023 22:47, Jeff King wrote:
> In sequencer_get_last_command(), we don't ever look at the repository
> parameter. It _should_ be used when calling into git_path_* functions,
> but the one we use here is declared with the non-REPO variant of
> GIT_PATH_FUNC(), and so just uses the_repository internally.
> 
> We could change the path helper to use REPO_GIT_PATH_FUNC(), but doing
> so piecemeal is not great. There are 41 uses of GIT_PATH_FUNC() in
> sequencer.c,

Wow, I knew there were quite a few but I hadn't realized there were that 
many. Changing them all to take a struct repository will be a big change 
and will make struct repo_cache_path a lot larger.

> and inconsistently switching one makes the code more
> confusing. Likewise, this one function is used in half a dozen other
> spots, all of which would need to start passing in a repository argument
> (with rippling effects up the call stack).
> 
> So let's punt on that for now and just silence any -Wunused-parameter
> warning.
> 
> Note that we could also drop this parameter entirely, as the function is
> always called directly, and not as a callback that has to conform to
> some external interface. But since we'd eventually want to use the
> repository parameter, let's leave it in place to avoid disrupting the
> callers twice.

I think that makes sense as we're going to need that argument 
eventually. I was curious as to why this function takes a repository 
argument. When the function was added in 4a72486de97 (fix 
cherry-pick/revert status after commit, 2019-04-16) it called 
parse_insn_line() which takes a repository argument. It was refactored 
in ed5b1ca10b (status: do not report errors in sequencer/todo, 
2019-06-27) and I failed to notice that the repository was unused 
afterwards.

Best Wishes

Phillip


> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   sequencer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 82dc3e160e..41fd79d215 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2649,7 +2649,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>   	return item->commit ? 0 : -1;
>   }
>   
> -int sequencer_get_last_command(struct repository *r, enum replay_action *action)
> +int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
>   {
>   	const char *todo_file, *bol;
>   	struct strbuf buf = STRBUF_INIT;

Re: [PATCH 02/22] sequencer: mark repository argument as unused

[Jeff King]

On Tue, Aug 29, 2023 at 04:55:30PM +0100, Phillip Wood wrote:

> > We could change the path helper to use REPO_GIT_PATH_FUNC(), but doing
> > so piecemeal is not great. There are 41 uses of GIT_PATH_FUNC() in
> > sequencer.c,
> 
> Wow, I knew there were quite a few but I hadn't realized there were that
> many. Changing them all to take a struct repository will be a big change and
> will make struct repo_cache_path a lot larger.

Yeah. One of the things that gave me pause is that some of them may need
to be renamed, as well. Most of the existing ones are static local
within sequencer.c, so names like git_path_head_file() are OK. But
REPO_GIT_PATH_FUNC() requires a pointer in the global repo_cache_path,
so the names need to be appropriate for a global view.

> > Note that we could also drop this parameter entirely, as the function is
> > always called directly, and not as a callback that has to conform to
> > some external interface. But since we'd eventually want to use the
> > repository parameter, let's leave it in place to avoid disrupting the
> > callers twice.
> 
> I think that makes sense as we're going to need that argument eventually. I
> was curious as to why this function takes a repository argument. When the
> function was added in 4a72486de97 (fix cherry-pick/revert status after
> commit, 2019-04-16) it called parse_insn_line() which takes a repository
> argument. It was refactored in ed5b1ca10b (status: do not report errors in
> sequencer/todo, 2019-06-27) and I failed to notice that the repository was
> unused afterwards.

Thanks for digging there. I usually do something similar for these
patches (to help verify that there's no extra bug lurking), but with
"struct repository" ones, the answer was usually uninteresting ("it
should be used, but the underlying functions don't support it" kind of
thing).

Since I'm re-rolling anyway, I'll throw your research into the commit
message.

-Peff

Re: [PATCH 02/22] sequencer: mark repository argument as unused

[Phillip Wood]

On 30/08/2023 00:32, Jeff King wrote:
> On Tue, Aug 29, 2023 at 04:55:30PM +0100, Phillip Wood wrote:
> 
>>> We could change the path helper to use REPO_GIT_PATH_FUNC(), but doing
>>> so piecemeal is not great. There are 41 uses of GIT_PATH_FUNC() in
>>> sequencer.c,
>>
>> Wow, I knew there were quite a few but I hadn't realized there were that
>> many. Changing them all to take a struct repository will be a big change and
>> will make struct repo_cache_path a lot larger.
> 
> Yeah. One of the things that gave me pause is that some of them may need
> to be renamed, as well. Most of the existing ones are static local
> within sequencer.c, so names like git_path_head_file() are OK. But
> REPO_GIT_PATH_FUNC() requires a pointer in the global repo_cache_path,
> so the names need to be appropriate for a global view.

It's definitely worth leaving that for a separate patch series. A lot of 
the paths start rebase_path_... (I think it is only the paths used by 
cherry-pick/revert that start git_path_...) so we'd need a separate 
macro unless we renamed them. Having a different prefix is probably a 
good thing for making them global functions but it still leaves the 
problem of renaming the ones like git_path_head_file().

Best Wishes

Phillip

[PATCH 03/22] ref-filter: mark unused parameters in parser callbacks

[Jeff King]

These are similar to the cases annotated in 5fe9e1ce2f (ref-filter: mark
unused callback parameters, 2023-02-24), but were added after that
commit.

Note that the ahead/behind callback ignores its "atom" parameter, which
is a little unusual, since that struct usually stores the result. But in
this case, the data is stored centrally in ref_array->counts, since we
want to compute all ahead/behinds at once, not per ref.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1bfaf20fbf..88b021dd1d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -549,7 +549,8 @@ static int signature_atom_parser(struct ref_format *format UNUSED,
 	return 0;
 }
 
-static int trailers_atom_parser(struct ref_format *format, struct used_atom *atom,
+static int trailers_atom_parser(struct ref_format *format UNUSED,
+				struct used_atom *atom,
 				const char *arg, struct strbuf *err)
 {
 	atom->u.contents.trailer_opts.no_divider = 1;
@@ -819,7 +820,7 @@ static int if_atom_parser(struct ref_format *format UNUSED,
 	return 0;
 }
 
-static int rest_atom_parser(struct ref_format *format,
+static int rest_atom_parser(struct ref_format *format UNUSED,
 			    struct used_atom *atom UNUSED,
 			    const char *arg, struct strbuf *err)
 {
@@ -828,7 +829,8 @@ static int rest_atom_parser(struct ref_format *format,
 	return 0;
 }
 
-static int ahead_behind_atom_parser(struct ref_format *format, struct used_atom *atom,
+static int ahead_behind_atom_parser(struct ref_format *format,
+				    struct used_atom *atom UNUSED,
 				    const char *arg, struct strbuf *err)
 {
 	struct string_list_item *item;
-- 
2.42.0.505.g4c6fb48dec

[PATCH 04/22] pack-bitmap: mark unused parameters in show_object callback

[Jeff King]

This is similar to the cases in c50dca2a18 (list-objects: mark unused
callback parameters, 2023-02-24), but was added after that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 pack-bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 6afc03d1e4..ca8319b87c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1101,8 +1101,9 @@ static void show_boundary_commit(struct commit *commit, void *_data)
 	}
 }
 
-static void show_boundary_object(struct object *object,
-				 const char *name, void *data)
+static void show_boundary_object(struct object *object UNUSED,
+				 const char *name UNUSED,
+				 void *data UNUSED)
 {
 	BUG("should not be called");
 }
-- 
2.42.0.505.g4c6fb48dec

[PATCH 05/22] worktree: mark unused parameters in each_ref_fn callback

[Jeff King]

This is similar to the cases in 63e14ee2d6 (refs: mark unused
each_ref_fn parameters, 2022-08-19), but it was added after that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 10db70b7ec..62b7e26f4b 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -628,10 +628,10 @@ static void print_preparing_worktree_line(int detach,
  *
  * Returns 0 on failure and non-zero on success.
  */
-static int first_valid_ref(const char *refname,
-			   const struct object_id *oid,
-			   int flags,
-			   void *cb_data)
+static int first_valid_ref(const char *refname UNUSED,
+			   const struct object_id *oid UNUSED,
+			   int flags UNUSED,
+			   void *cb_data UNUSED)
 {
 	return 1;
 }
-- 
2.42.0.505.g4c6fb48dec

[PATCH 06/22] commit-graph: mark unused data parameters in generation callbacks

[Jeff King]

The compute_generation_info code uses function pointers to abstract the
get/set generation operations. Some callers don't need the extra void
data pointer, which should be annotated to appease -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 0aa1640d15..73a539495b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1568,12 +1568,14 @@ static void compute_topological_levels(struct write_commit_graph_context *ctx)
 	stop_progress(&ctx->progress);
 }
 
-static timestamp_t get_generation_from_graph_data(struct commit *c, void *data)
+static timestamp_t get_generation_from_graph_data(struct commit *c,
+						  void *data UNUSED)
 {
 	return commit_graph_data_at(c)->generation;
 }
 
-static void set_generation_v2(struct commit *c, timestamp_t t, void *data)
+static void set_generation_v2(struct commit *c, timestamp_t t,
+			      void *data UNUSED)
 {
 	struct commit_graph_data *g = commit_graph_data_at(c);
 	g->generation = t;
@@ -1616,7 +1618,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 }
 
 static void set_generation_in_graph_data(struct commit *c, timestamp_t t,
-					 void *data)
+					 void *data UNUSED)
 {
 	commit_graph_data_at(c)->generation = t;
 }
-- 
2.42.0.505.g4c6fb48dec

Re: [PATCH 06/22] commit-graph: mark unused data parameters in generation callbacks

[Taylor Blau]

On Mon, Aug 28, 2023 at 05:47:39PM -0400, Jeff King wrote:
> The compute_generation_info code uses function pointers to abstract the
> get/set generation operations. Some callers don't need the extra void
> data pointer, which should be annotated to appease -Wunused-parameter.

I think just the callbacks initialized by compute_topological_levels()
(i.e, get_topo_level() and set_topo_level()) care about the ctx
parameter.

I think that we can go a bit further, though. The other caller in
compute_generation_numbers() assigns data to the argument ctx it takes
in, but neither of its callbacks get_generation_from_graph_data() and
set_generation_v2() use the data parameter, as is implied by the the
existence of this patch.

So I think that we could drop the assignment to data entirely like so,
applied on top of this patch:

--- 8< ---
diff --git a/commit-graph.c b/commit-graph.c
index 0aa1640d15..eb3e27b720 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1587,7 +1587,6 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 		.commits = &ctx->commits,
 		.get_generation = get_generation_from_graph_data,
 		.set_generation = set_generation_v2,
-		.data = ctx,
 	};

 	if (ctx->report_progress)
--- >8 ---

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  commit-graph.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

The rest looks obviously correct to me, thanks.

Thanks,
Taylor

Re: [PATCH 06/22] commit-graph: mark unused data parameters in generation callbacks

[Jeff King]

On Mon, Aug 28, 2023 at 07:32:59PM -0400, Taylor Blau wrote:

> On Mon, Aug 28, 2023 at 05:47:39PM -0400, Jeff King wrote:
> > The compute_generation_info code uses function pointers to abstract the
> > get/set generation operations. Some callers don't need the extra void
> > data pointer, which should be annotated to appease -Wunused-parameter.
> 
> I think just the callbacks initialized by compute_topological_levels()
> (i.e, get_topo_level() and set_topo_level()) care about the ctx
> parameter.
> 
> I think that we can go a bit further, though. The other caller in
> compute_generation_numbers() assigns data to the argument ctx it takes
> in, but neither of its callbacks get_generation_from_graph_data() and
> set_generation_v2() use the data parameter, as is implied by the the
> existence of this patch.
> 
> So I think that we could drop the assignment to data entirely like so,
> applied on top of this patch:
> 
> --- 8< ---
> diff --git a/commit-graph.c b/commit-graph.c
> index 0aa1640d15..eb3e27b720 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1587,7 +1587,6 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
>  		.commits = &ctx->commits,
>  		.get_generation = get_generation_from_graph_data,
>  		.set_generation = set_generation_v2,
> -		.data = ctx,
>  	};
> 
>  	if (ctx->report_progress)
> --- >8 ---

Yeah, I didn't dig too much in the caller. I could see an argument for
leaving it, in that it might be the "least surprise" for somebody who
later wants to look at the ctx variable from those callbacks. But since
all they get is a void pointer, anybody writing that code would have to
go back to the caller to see what is passed in as "data" anyway.

So I think it is safe to add this cleanup. I'm going to re-roll to
simplify the first patch a bit, so I'll add this in as well. Thanks.

-Peff

[PATCH 07/22] ls-tree: mark unused parameter in callback

[Jeff King]

The formatting functions are dispatched from a table of function
pointers. The "path name only" function unsurprisingly does not need to
look at its "oid" parameter, but we must mark it as unused to make
-Wunused-parameter happy.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/ls-tree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index f558db5f3b..209d2dc0d5 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -241,7 +241,8 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base,
 	return recurse;
 }
 
-static int show_tree_name_only(const struct object_id *oid, struct strbuf *base,
+static int show_tree_name_only(const struct object_id *oid UNUSED,
+			       struct strbuf *base,
 			       const char *pathname, unsigned mode,
 			       void *context)
 {
-- 
2.42.0.505.g4c6fb48dec

[PATCH 08/22] stash: mark unused parameter in diff callback

[Jeff King]

This is similar to the cases in 61bdc7c5d8 (diff: mark unused parameters
in callbacks, 2022-12-13), but I missed it when making that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/stash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index fe64cde9ce..c9365bea13 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -362,7 +362,7 @@ static int is_path_a_directory(const char *path)
 }
 
 static void add_diff_to_buf(struct diff_queue_struct *q,
-			    struct diff_options *options,
+			    struct diff_options *options UNUSED,
 			    void *data)
 {
 	int i;
-- 
2.42.0.505.g4c6fb48dec

[PATCH 09/22] trace2: mark unused us_elapsed_absolute parameters

[Jeff King]

Many trace2 targets ignore the absolute elapsed time parameters.
However, the virtual interface needs to retain the parameter since it is
used by others (e.g., the perf target).

Signed-off-by: Jeff King <peff@peff.net>
---
 trace2/tr2_tgt_event.c  | 23 +++++++++++++----------
 trace2/tr2_tgt_normal.c | 20 ++++++++++++--------
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 53091781ec..59910a1a4f 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -335,7 +335,7 @@ static void fn_alias_fl(const char *file, int line, const char *alias,
 }
 
 static void fn_child_start_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute,
+			      uint64_t us_elapsed_absolute UNUSED,
 			      const struct child_process *cmd)
 {
 	const char *event_name = "child_start";
@@ -367,7 +367,8 @@ static void fn_child_start_fl(const char *file, int line,
 }
 
 static void fn_child_exit_fl(const char *file, int line,
-			     uint64_t us_elapsed_absolute, int cid, int pid,
+			     uint64_t us_elapsed_absolute UNUSED,
+			     int cid, int pid,
 			     int code, uint64_t us_elapsed_child)
 {
 	const char *event_name = "child_exit";
@@ -388,7 +389,8 @@ static void fn_child_exit_fl(const char *file, int line,
 }
 
 static void fn_child_ready_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int cid, int pid,
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int cid, int pid,
 			      const char *ready, uint64_t us_elapsed_child)
 {
 	const char *event_name = "child_ready";
@@ -409,7 +411,7 @@ static void fn_child_ready_fl(const char *file, int line,
 }
 
 static void fn_thread_start_fl(const char *file, int line,
-			       uint64_t us_elapsed_absolute)
+			       uint64_t us_elapsed_absolute UNUSED)
 {
 	const char *event_name = "thread_start";
 	struct json_writer jw = JSON_WRITER_INIT;
@@ -423,7 +425,7 @@ static void fn_thread_start_fl(const char *file, int line,
 }
 
 static void fn_thread_exit_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute,
+			      uint64_t us_elapsed_absolute UNUSED,
 			      uint64_t us_elapsed_thread)
 {
 	const char *event_name = "thread_exit";
@@ -439,7 +441,8 @@ static void fn_thread_exit_fl(const char *file, int line,
 	jw_release(&jw);
 }
 
-static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
+static void fn_exec_fl(const char *file, int line,
+		       uint64_t us_elapsed_absolute UNUSED,
 		       int exec_id, const char *exe, const char **argv)
 {
 	const char *event_name = "exec";
@@ -460,8 +463,8 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 }
 
 static void fn_exec_result_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int exec_id,
-			      int code)
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int exec_id, int code)
 {
 	const char *event_name = "exec_result";
 	struct json_writer jw = JSON_WRITER_INIT;
@@ -511,7 +514,7 @@ static void fn_repo_fl(const char *file, int line,
 }
 
 static void fn_region_enter_printf_va_fl(const char *file, int line,
-					 uint64_t us_elapsed_absolute,
+					 uint64_t us_elapsed_absolute UNUSED,
 					 const char *category,
 					 const char *label,
 					 const struct repository *repo,
@@ -538,7 +541,7 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,
 }
 
 static void fn_region_leave_printf_va_fl(
-	const char *file, int line, uint64_t us_elapsed_absolute,
+	const char *file, int line, uint64_t us_elapsed_absolute UNUSED,
 	uint64_t us_elapsed_region, const char *category, const char *label,
 	const struct repository *repo, const char *fmt, va_list ap)
 {
diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index d25ea13164..38d5ebddf6 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -86,7 +86,7 @@ static void fn_version_fl(const char *file, int line)
 }
 
 static void fn_start_fl(const char *file, int line,
-			uint64_t us_elapsed_absolute, const char **argv)
+			uint64_t us_elapsed_absolute UNUSED, const char **argv)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
 
@@ -215,7 +215,7 @@ static void fn_alias_fl(const char *file, int line, const char *alias,
 }
 
 static void fn_child_start_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute,
+			      uint64_t us_elapsed_absolute UNUSED,
 			      const struct child_process *cmd)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -243,7 +243,8 @@ static void fn_child_start_fl(const char *file, int line,
 }
 
 static void fn_child_exit_fl(const char *file, int line,
-			     uint64_t us_elapsed_absolute, int cid, int pid,
+			     uint64_t us_elapsed_absolute UNUSED,
+			     int cid, int pid,
 			     int code, uint64_t us_elapsed_child)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -256,7 +257,8 @@ static void fn_child_exit_fl(const char *file, int line,
 }
 
 static void fn_child_ready_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int cid, int pid,
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int cid, int pid,
 			      const char *ready, uint64_t us_elapsed_child)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -268,7 +270,8 @@ static void fn_child_ready_fl(const char *file, int line,
 	strbuf_release(&buf_payload);
 }
 
-static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
+static void fn_exec_fl(const char *file, int line,
+		       uint64_t us_elapsed_absolute UNUSED,
 		       int exec_id, const char *exe, const char **argv)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -284,8 +287,8 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 }
 
 static void fn_exec_result_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int exec_id,
-			      int code)
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int exec_id, int code)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
 
@@ -321,7 +324,8 @@ static void fn_repo_fl(const char *file, int line,
 }
 
 static void fn_printf_va_fl(const char *file, int line,
-			    uint64_t us_elapsed_absolute, const char *fmt,
+			    uint64_t us_elapsed_absolute UNUSED,
+			    const char *fmt,
 			    va_list ap)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
-- 
2.42.0.505.g4c6fb48dec

[PATCH 10/22] trace2: mark unused config callback parameter

[Jeff King]

This should have been part of 783a86c142 (config: mark unused callback
parameters, 2022-08-19), but was missed in that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 trace2/tr2_sysenv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index f26ec95ab4..d3ecac2772 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -58,7 +58,8 @@ static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
 /* clang-format on */
 
 static int tr2_sysenv_cb(const char *key, const char *value,
-			 const struct config_context *ctx UNUSED, void *d)
+			 const struct config_context *ctx UNUSED,
+			 void *d UNUSED)
 {
 	int k;
 
-- 
2.42.0.505.g4c6fb48dec

[PATCH 11/22] test-trace2: mark unused argv/argc parameters

[Jeff King]

The trace2 test helper uses function pointers to dispatch to individual
tests. Not all tests bother looking at their argv/argc parameters. We
could tighten this up (e.g., complaining when seeing unexpected
parameters), but for internal test code it's not worth worrying about.

This is similar in spirit to 126e3b3d2a (t/helper: mark unused argv/argc
arguments, 2023-03-28).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-trace2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index 20c7495f38..d5ca0046c8 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -45,7 +45,7 @@ static int get_i(int *p_value, const char *data)
  * [] "def_param" events for all of the "interesting" pre-defined
  * config settings.
  */
-static int ut_001return(int argc, const char **argv)
+static int ut_001return(int argc UNUSED, const char **argv)
 {
 	int rc;
 
@@ -65,7 +65,7 @@ static int ut_001return(int argc, const char **argv)
  * [] "def_param" events for all of the "interesting" pre-defined
  * config settings.
  */
-static int ut_002exit(int argc, const char **argv)
+static int ut_002exit(int argc UNUSED, const char **argv)
 {
 	int rc;
 
@@ -201,7 +201,7 @@ static int ut_006data(int argc, const char **argv)
 	return 0;
 }
 
-static int ut_007BUG(int argc, const char **argv)
+static int ut_007BUG(int argc UNUSED, const char **argv UNUSED)
 {
 	/*
 	 * Exercise BUG() to ensure that the message is printed to trace2.
-- 
2.42.0.505.g4c6fb48dec

[PATCH 12/22] grep: mark unused parameter in output function

[Jeff King]

This is a callback used with grep_options.output, but we don't look at
the grep_opt parameter, as we're just writing the output to stdout.

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index 0904d55b24..0124eb1960 100644
--- a/grep.c
+++ b/grep.c
@@ -17,7 +17,7 @@ static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs,
 				 struct index_state *istate);
 
-static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+static void std_output(struct grep_opt *opt UNUSED, const void *buf, size_t size)
 {
 	fwrite(buf, size, 1, stdout);
 }
-- 
2.42.0.505.g4c6fb48dec

[PATCH 13/22] add-interactive: mark unused callback parameters

[Jeff King]

The interactive commands are dispatched from a table of abstract
pointers, but not every command uses every parameter it receives. Mark
the unused ones to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 add-interactive.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index add9a1ad43..852e8e6b2f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1021,9 +1021,9 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
 	return res;
 }
 
-static int run_help(struct add_i_state *s, const struct pathspec *unused_ps,
-		    struct prefix_item_list *unused_files,
-		    struct list_and_choose_options *unused_opts)
+static int run_help(struct add_i_state *s, const struct pathspec *ps UNUSED,
+		    struct prefix_item_list *files UNUSED,
+		    struct list_and_choose_options *opts UNUSED)
 {
 	color_fprintf_ln(stdout, s->help_color, "status        - %s",
 			 _("show paths with changes"));
@@ -1074,7 +1074,7 @@ struct print_command_item_data {
 	const char *color, *reset;
 };
 
-static void print_command_item(int i, int selected,
+static void print_command_item(int i, int selected UNUSED,
 			       struct string_list_item *item,
 			       void *print_command_item_data)
 {
-- 
2.42.0.505.g4c6fb48dec

[PATCH 14/22] negotiator/noop: mark unused callback parameters

[Jeff King]

The noop negotiator unsurprisingly does not bother looking at any of its
parameters. Mark them unused to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 negotiator/noop.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/negotiator/noop.c b/negotiator/noop.c
index 7b72937686..de39028ab7 100644
--- a/negotiator/noop.c
+++ b/negotiator/noop.c
@@ -3,22 +3,24 @@
 #include "../commit.h"
 #include "../fetch-negotiator.h"
 
-static void known_common(struct fetch_negotiator *n, struct commit *c)
+static void known_common(struct fetch_negotiator *n UNUSED,
+			 struct commit *c UNUSED)
 {
 	/* do nothing */
 }
 
-static void add_tip(struct fetch_negotiator *n, struct commit *c)
+static void add_tip(struct fetch_negotiator *n UNUSED,
+		    struct commit *c UNUSED)
 {
 	/* do nothing */
 }
 
-static const struct object_id *next(struct fetch_negotiator *n)
+static const struct object_id *next(struct fetch_negotiator *n UNUSED)
 {
 	return NULL;
 }
 
-static int ack(struct fetch_negotiator *n, struct commit *c)
+static int ack(struct fetch_negotiator *n UNUSED, struct commit *c UNUSED)
 {
 	/*
 	 * This negotiator does not emit any commits, so there is no commit to
@@ -28,7 +30,7 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
 	return 0;
 }
 
-static void release(struct fetch_negotiator *n)
+static void release(struct fetch_negotiator *n UNUSED)
 {
 	/* nothing to release */
 }
-- 
2.42.0.505.g4c6fb48dec

[PATCH 15/22] worktree: mark unused parameters in noop repair callback

[Jeff King]

The noop repair callback unsurprisingly does not look at any of its
parameters. Mark them as unused to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 worktree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/worktree.c b/worktree.c
index b8cf29e6a1..a56a6c2a3d 100644
--- a/worktree.c
+++ b/worktree.c
@@ -581,8 +581,10 @@ static void repair_gitfile(struct worktree *wt,
 	strbuf_release(&dotgit);
 }
 
-static void repair_noop(int iserr, const char *path, const char *msg,
-			void *cb_data)
+static void repair_noop(int iserr UNUSED,
+			const char *path UNUSED,
+			const char *msg UNUSED,
+			void *cb_data UNUSED)
 {
 	/* nothing */
 }
-- 
2.42.0.505.g4c6fb48dec

[PATCH 16/22] imap-send: mark unused parameters with NO_OPENSSL

[Jeff King]

Earlier patches annotating unused parameters in imap-send missed a few
cases in code that is compiled only with NO_OPENSSL. These need to
retain the extra parameters to match the interfaces used when we compile
with openssl support.

Note in the case of socket_perror() that the function declaration and
parts of its code are shared between the two cases, and only the openssl
code looks at "sock". So we can't simply mark the parameter as always
unused. Instead, we can add a noop statement that references it. This is
ugly, but should be portable.

Signed-off-by: Jeff King <peff@peff.net>
---
 imap-send.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 06386e0b3b..996651e4f8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -206,10 +206,14 @@ static void socket_perror(const char *func, struct imap_socket *sock, int ret)
 		else
 			fprintf(stderr, "%s: unexpected EOF\n", func);
 	}
+	/* mark as used to appease -Wunused-parameter with NO_OPENSSL */
+	(void)sock;
 }
 
 #ifdef NO_OPENSSL
-static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int verify)
+static int ssl_socket_connect(struct imap_socket *sock UNUSED,
+			      int use_tls_only UNUSED,
+			      int verify UNUSED)
 {
 	fprintf(stderr, "SSL requested but SSL support not compiled in\n");
 	return -1;
@@ -904,7 +908,9 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
 
 #else
 
-static char *cram(const char *challenge_64, const char *user, const char *pass)
+static char *cram(const char *challenge_64 UNUSED,
+		  const char *user UNUSED,
+		  const char *pass UNUSED)
 {
 	die("If you want to use CRAM-MD5 authenticate method, "
 	    "you have to build git-imap-send with OpenSSL library.");
-- 
2.42.0.505.g4c6fb48dec

[PATCH 17/22] grep: mark unused parmaeters in pcre fallbacks

[Jeff King]

When USE_LIBPCRE2 is not defined, we compile several noop fallbacks.
These need to have their parameters annotated to avoid
-Wunused-parameter warnings (and obviously we cannot remove the
parameters, since the functions must match the non-fallback versions).

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 0124eb1960..fc2d0c837a 100644
--- a/grep.c
+++ b/grep.c
@@ -452,18 +452,20 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_general_context_free(p->pcre2_general_context);
 }
 #else /* !USE_LIBPCRE2 */
-static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
+static void compile_pcre2_pattern(struct grep_pat *p UNUSED,
+				  const struct grep_opt *opt UNUSED)
 {
 	die("cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE");
 }
 
-static int pcre2match(struct grep_pat *p, const char *line, const char *eol,
-		regmatch_t *match, int eflags)
+static int pcre2match(struct grep_pat *p UNUSED, const char *line UNUSED,
+		      const char *eol UNUSED, regmatch_t *match UNUSED,
+		      int eflags UNUSED)
 {
 	return 1;
 }
 
-static void free_pcre2_pattern(struct grep_pat *p)
+static void free_pcre2_pattern(struct grep_pat *p UNUSED)
 {
 }
 
-- 
2.42.0.505.g4c6fb48dec

[PATCH 18/22] credential: mark unused parameter in urlmatch callback

[Jeff King]

Our select_all() callback does not need to actually look at its
parameters, since the point is to match everything. But we need to mark
its parameters to satisfy -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/credential.c b/credential.c
index d664754163..18098bd35e 100644
--- a/credential.c
+++ b/credential.c
@@ -88,8 +88,8 @@ static int proto_is_http(const char *s)
 static void credential_describe(struct credential *c, struct strbuf *out);
 static void credential_format(struct credential *c, struct strbuf *out);
 
-static int select_all(const struct urlmatch_item *a,
-		      const struct urlmatch_item *b)
+static int select_all(const struct urlmatch_item *a UNUSED,
+		      const struct urlmatch_item *b UNUSED)
 {
 	return 0;
 }
-- 
2.42.0.505.g4c6fb48dec

[PATCH 19/22] fetch: mark unused parameter in ref_transaction callback

[Jeff King]

Since this callback is just trying to collect the set of queued tag
updates, there is no need for it to look at old_oid at all. Mark it as
unused to appease -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index eed4a7cdb6..8f93529505 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -308,7 +308,7 @@ static void clear_item(struct refname_hash_entry *item)
 
 
 static void add_already_queued_tags(const char *refname,
-				    const struct object_id *old_oid,
+				    const struct object_id *old_oid UNUSED,
 				    const struct object_id *new_oid,
 				    void *cb_data)
 {
-- 
2.42.0.505.g4c6fb48dec

[PATCH 20/22] bundle-uri: mark unused parameters in callbacks

[Jeff King]

The first hunk is similar to 02c3c59e62 (hashmap: mark unused callback
parameters, 2022-08-19), but was added after that commit.

The other two are used with for_all_bundles_in_list(), but don't use
their void data pointer.

Signed-off-by: Jeff King <peff@peff.net>
---
 bundle-uri.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bundle-uri.c b/bundle-uri.c
index 4b5c49b93d..8492fffd2f 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -20,7 +20,7 @@ static struct {
 	{ BUNDLE_HEURISTIC_CREATIONTOKEN, "creationToken" },
 };
 
-static int compare_bundles(const void *hashmap_cmp_fn_data,
+static int compare_bundles(const void *hashmap_cmp_fn_data UNUSED,
 			   const struct hashmap_entry *he1,
 			   const struct hashmap_entry *he2,
 			   const void *id)
@@ -45,7 +45,7 @@ void init_bundle_list(struct bundle_list *list)
 }
 
 static int clear_remote_bundle_info(struct remote_bundle_info *bundle,
-				    void *data)
+				    void *data UNUSED)
 {
 	FREE_AND_NULL(bundle->id);
 	FREE_AND_NULL(bundle->uri);
@@ -779,7 +779,7 @@ static int unbundle_all_bundles(struct repository *r,
 	return 0;
 }
 
-static int unlink_bundle(struct remote_bundle_info *info, void *data)
+static int unlink_bundle(struct remote_bundle_info *info, void *data UNUSED)
 {
 	if (info->file)
 		unlink_or_warn(info->file);
-- 
2.42.0.505.g4c6fb48dec

[PATCH 21/22] gc: mark unused descriptors in scheduler callbacks

[Jeff King]

Each of the scheduler update callbacks gets the descriptor of the lock
file, but only the crontab updater needs it. We have to retain the
unused descriptors because these are dispatched from a table of function
pointers, but we should mark them to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/gc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 1f53b66c7b..369bd43fb2 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1934,7 +1934,7 @@ static int launchctl_add_plists(void)
 	       launchctl_schedule_plist(exec_path, SCHEDULE_WEEKLY);
 }
 
-static int launchctl_update_schedule(int run_maintenance, int fd)
+static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)
 {
 	if (run_maintenance)
 		return launchctl_add_plists();
@@ -2115,7 +2115,7 @@ static int schtasks_schedule_tasks(void)
 	       schtasks_schedule_task(exec_path, SCHEDULE_WEEKLY);
 }
 
-static int schtasks_update_schedule(int run_maintenance, int fd)
+static int schtasks_update_schedule(int run_maintenance, int fd UNUSED)
 {
 	if (run_maintenance)
 		return schtasks_schedule_tasks();
@@ -2556,7 +2556,7 @@ static int systemd_timer_setup_units(void)
 	return ret;
 }
 
-static int systemd_timer_update_schedule(int run_maintenance, int fd)
+static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
 {
 	if (run_maintenance)
 		return systemd_timer_setup_units();
-- 
2.42.0.505.g4c6fb48dec

[PATCH 22/22] update-ref: mark unused parameter in parser callbacks

[Jeff King]

The parsing of stdin is driven by a table of function pointers; mark
unused parameters in concrete functions to avoid -Wunused-parameter
warnings.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/update-ref.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 242102273e..c0c4e65e6f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -311,8 +311,8 @@ static void report_ok(const char *command)
 	fflush(stdout);
 }
 
-static void parse_cmd_option(struct ref_transaction *transaction,
-			     const char *next, const char *end)
+static void parse_cmd_option(struct ref_transaction *transaction UNUSED,
+			     const char *next, const char *end UNUSED)
 {
 	const char *rest;
 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
@@ -321,16 +321,16 @@ static void parse_cmd_option(struct ref_transaction *transaction,
 		die("option unknown: %s", next);
 }
 
-static void parse_cmd_start(struct ref_transaction *transaction,
-			    const char *next, const char *end)
+static void parse_cmd_start(struct ref_transaction *transaction UNUSED,
+			    const char *next, const char *end UNUSED)
 {
 	if (*next != line_termination)
 		die("start: extra input: %s", next);
 	report_ok("start");
 }
 
 static void parse_cmd_prepare(struct ref_transaction *transaction,
-			      const char *next, const char *end)
+			      const char *next, const char *end UNUSED)
 {
 	struct strbuf error = STRBUF_INIT;
 	if (*next != line_termination)
@@ -341,7 +341,7 @@ static void parse_cmd_prepare(struct ref_transaction *transaction,
 }
 
 static void parse_cmd_abort(struct ref_transaction *transaction,
-			    const char *next, const char *end)
+			    const char *next, const char *end UNUSED)
 {
 	struct strbuf error = STRBUF_INIT;
 	if (*next != line_termination)
@@ -352,7 +352,7 @@ static void parse_cmd_abort(struct ref_transaction *transaction,
 }
 
 static void parse_cmd_commit(struct ref_transaction *transaction,
-			     const char *next, const char *end)
+			     const char *next, const char *end UNUSED)
 {
 	struct strbuf error = STRBUF_INIT;
 	if (*next != line_termination)
-- 
2.42.0.505.g4c6fb48dec

Re: [PATCH 0/22] YAUPS: Yet Another Unused Parameter Series

[Taylor Blau]

On Mon, Aug 28, 2023 at 05:46:04PM -0400, Jeff King wrote:
> I'm back with another exciting installment of -Wunused-parameter warning
> fixes. Most of these are pretty boring and obvious; the first two are
> the most interesting in terms of rationale.
>
> I promise we're closing in on the finish line here. I only have about 20
> patches left after this, at which point we should be able to turn on the
> warning by default for developer builds.

Heh ;-). It's good to see us getting dangerously close to being able to
turn on -Wunused-parameter in the DEVELOPER builds.

I reviewed this series, and all looked sensible. I left a couple notes
throughout for potential further cleanups that we could do on top, which
might be nice to squash in.

But I wouldn't be heartbroken if you want to ignore them, either. So
with or without my suggestions, this series LGTM.

Thanks,
Taylor

[PATCH v2 0/22] Yet Another Unused Parameter Series

[Jeff King]

On Mon, Aug 28, 2023 at 05:46:04PM -0400, Jeff King wrote:

> I'm back with another exciting installment of -Wunused-parameter warning
> fixes. Most of these are pretty boring and obvious; the first two are
> the most interesting in terms of rationale.
> 
> I promise we're closing in on the finish line here. I only have about 20
> patches left after this, at which point we should be able to turn on the
> warning by default for developer builds.

And here's a v2 based on feedback from round 1. The changes are small,
so I'll let the range-diff below speak for itself.

 1:  6009723d8a !  1:  c11e9b9bd6 sequencer: use repository parameter in short_commit_name()
    @@ Commit message
         the_repository, but this pushes us one small step forward to eventually
         getting rid of those.
     
    +    Note that a few of these functions have a "rev_info" whose "repo"
    +    parameter could probably be used instead of the_repository. I'm leaving
    +    that for further cleanups, as it's not immediately obvious that
    +    revs->repo is always valid, and there's quite a bit of other possible
    +    refactoring here (even getting rid of some "struct repository" arguments
    +    in favor of revs->repo).
    +
         Signed-off-by: Jeff King <peff@peff.net>
     
      ## sequencer.c ##
    @@ sequencer.c: static int walk_revs_populate_todo(struct todo_list *todo_list,
      		subject_len = find_commit_subject(commit_buffer, &subject);
      		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
     -			short_commit_name(commit), subject_len, subject);
    -+			short_commit_name(opts->revs->repo, commit),
    ++			short_commit_name(the_repository, commit),
     +			subject_len, subject);
      		repo_unuse_commit_buffer(the_repository, commit,
      					 commit_buffer);
    @@ sequencer.c: static int make_script_with_merges(struct pretty_print_context *pp,
      			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
      				warning(_("skipped previously applied commit %s"),
     -					short_commit_name(commit));
    -+					short_commit_name(revs->repo, commit));
    ++					short_commit_name(the_repository, commit));
      			skipped_commit = 1;
      			continue;
      		}
 2:  f9068919f9 !  2:  3d0030ed7a sequencer: mark repository argument as unused
    @@ Commit message
         sequencer: mark repository argument as unused
     
         In sequencer_get_last_command(), we don't ever look at the repository
    -    parameter. It _should_ be used when calling into git_path_* functions,
    +    parameter. This is due to ed5b1ca10b (status: do not report errors in
    +    sequencer/todo, 2019-06-27), which dropped the call to parse_insn_line().
    +
    +    However, it _should_ be used when calling into git_path_* functions,
         but the one we use here is declared with the non-REPO variant of
         GIT_PATH_FUNC(), and so just uses the_repository internally.
     
 3:  cf28e8793a =  3:  deca060ede ref-filter: mark unused parameters in parser callbacks
 4:  baba1341f3 =  4:  4c15910f4e pack-bitmap: mark unused parameters in show_object callback
 5:  444d67676b =  5:  9125111e5e worktree: mark unused parameters in each_ref_fn callback
 6:  4d5fbed1aa !  6:  4e044c1005 commit-graph: mark unused data parameters in generation callbacks
    @@ Commit message
         get/set generation operations. Some callers don't need the extra void
         data pointer, which should be annotated to appease -Wunused-parameter.
     
    +    Note that we can drop the assignment of the "data" parameter in
    +    compute_generation_numbers(), as we've just shown that neither of the
    +    callbacks it uses will access it. This matches the caller in
    +    ensure_generations_valid(), which already does not bother to set "data".
    +
         Signed-off-by: Jeff King <peff@peff.net>
     
      ## commit-graph.c ##
    @@ commit-graph.c: static void compute_topological_levels(struct write_commit_graph
      {
      	struct commit_graph_data *g = commit_graph_data_at(c);
      	g->generation = t;
    +@@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph_context *ctx)
    + 		.commits = &ctx->commits,
    + 		.get_generation = get_generation_from_graph_data,
    + 		.set_generation = set_generation_v2,
    +-		.data = ctx,
    + 	};
    + 
    + 	if (ctx->report_progress)
     @@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph_context *ctx)
      }
      
 7:  b5ad1e7e6a =  7:  8441011ab3 ls-tree: mark unused parameter in callback
 8:  d0d9fb3c7e =  8:  4ee523564a stash: mark unused parameter in diff callback
 9:  eefe7daf5d =  9:  21a23b51a9 trace2: mark unused us_elapsed_absolute parameters
10:  38021bc8ce = 10:  76db35e8f1 trace2: mark unused config callback parameter
11:  a6eccfab8b = 11:  312c6bd59b test-trace2: mark unused argv/argc parameters
12:  6d971c4381 = 12:  e74362f5a8 grep: mark unused parameter in output function
13:  ac7347663c = 13:  a8525ab460 add-interactive: mark unused callback parameters
14:  d14aab5b51 = 14:  27df60c02f negotiator/noop: mark unused callback parameters
15:  2d405dc294 = 15:  4eeac4bc33 worktree: mark unused parameters in noop repair callback
16:  9c1f3b668b = 16:  d81464b1f1 imap-send: mark unused parameters with NO_OPENSSL
17:  ad63f999b9 = 17:  ea865a1077 grep: mark unused parmaeters in pcre fallbacks
18:  af414f38fc = 18:  335c42823b credential: mark unused parameter in urlmatch callback
19:  14694671eb = 19:  f06f36371b fetch: mark unused parameter in ref_transaction callback
20:  a375abd8fa = 20:  b1fe7e7e34 bundle-uri: mark unused parameters in callbacks
21:  611dc4d367 = 21:  c9e258e0fa gc: mark unused descriptors in scheduler callbacks
22:  0ec53b7a46 = 22:  7c2c8fd614 update-ref: mark unused parameter in parser callbacks

  [01/22]: sequencer: use repository parameter in short_commit_name()
  [02/22]: sequencer: mark repository argument as unused
  [03/22]: ref-filter: mark unused parameters in parser callbacks
  [04/22]: pack-bitmap: mark unused parameters in show_object callback
  [05/22]: worktree: mark unused parameters in each_ref_fn callback
  [06/22]: commit-graph: mark unused data parameters in generation callbacks
  [07/22]: ls-tree: mark unused parameter in callback
  [08/22]: stash: mark unused parameter in diff callback
  [09/22]: trace2: mark unused us_elapsed_absolute parameters
  [10/22]: trace2: mark unused config callback parameter
  [11/22]: test-trace2: mark unused argv/argc parameters
  [12/22]: grep: mark unused parameter in output function
  [13/22]: add-interactive: mark unused callback parameters
  [14/22]: negotiator/noop: mark unused callback parameters
  [15/22]: worktree: mark unused parameters in noop repair callback
  [16/22]: imap-send: mark unused parameters with NO_OPENSSL
  [17/22]: grep: mark unused parmaeters in pcre fallbacks
  [18/22]: credential: mark unused parameter in urlmatch callback
  [19/22]: fetch: mark unused parameter in ref_transaction callback
  [20/22]: bundle-uri: mark unused parameters in callbacks
  [21/22]: gc: mark unused descriptors in scheduler callbacks
  [22/22]: update-ref: mark unused parameter in parser callbacks

 add-interactive.c       |  8 ++++----
 builtin/fetch.c         |  2 +-
 builtin/gc.c            |  6 +++---
 builtin/ls-tree.c       |  3 ++-
 builtin/stash.c         |  2 +-
 builtin/update-ref.c    | 14 +++++++-------
 builtin/worktree.c      |  8 ++++----
 bundle-uri.c            |  6 +++---
 commit-graph.c          |  9 +++++----
 credential.c            |  4 ++--
 grep.c                  | 12 +++++++-----
 imap-send.c             | 10 ++++++++--
 negotiator/noop.c       | 12 +++++++-----
 pack-bitmap.c           |  5 +++--
 ref-filter.c            |  8 +++++---
 sequencer.c             | 27 ++++++++++++++-------------
 t/helper/test-trace2.c  |  6 +++---
 trace2/tr2_sysenv.c     |  3 ++-
 trace2/tr2_tgt_event.c  | 23 +++++++++++++----------
 trace2/tr2_tgt_normal.c | 20 ++++++++++++--------
 worktree.c              |  6 ++++--
 21 files changed, 110 insertions(+), 84 deletions(-)

[PATCH v2 01/22] sequencer: use repository parameter in short_commit_name()

[Jeff King]

Instead of just using the_repository, we can take a repository parameter
from the caller. Most of them already have one, and doing so clears up a
few -Wunused-parameter warnings. There are still a few callers which use
the_repository, but this pushes us one small step forward to eventually
getting rid of those.

Note that a few of these functions have a "rev_info" whose "repo"
parameter could probably be used instead of the_repository. I'm leaving
that for further cleanups, as it's not immediately obvious that
revs->repo is always valid, and there's quite a bit of other possible
refactoring here (even getting rid of some "struct repository" arguments
in favor of revs->repo).

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 48475d1cc6..cd5264171a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -433,10 +433,9 @@ struct commit_message {
 	const char *message;
 };
 
-static const char *short_commit_name(struct commit *commit)
+static const char *short_commit_name(struct repository *r, struct commit *commit)
 {
-	return repo_find_unique_abbrev(the_repository, &commit->object.oid,
-				       DEFAULT_ABBREV);
+	return repo_find_unique_abbrev(r, &commit->object.oid, DEFAULT_ABBREV);
 }
 
 static int get_message(struct commit *commit, struct commit_message *out)
@@ -446,7 +445,7 @@ static int get_message(struct commit *commit, struct commit_message *out)
 
 	out->message = repo_logmsg_reencode(the_repository, commit, NULL,
 					    get_commit_output_encoding());
-	abbrev = short_commit_name(commit);
+	abbrev = short_commit_name(the_repository, commit);
 
 	subject_len = find_commit_subject(out->message, &subject);
 
@@ -2383,7 +2382,7 @@ static int do_pick_commit(struct repository *r,
 		error(command == TODO_REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
-		      short_commit_name(commit), msg.subject);
+		      short_commit_name(r, commit), msg.subject);
 		print_advice(r, res == 1, opts);
 		repo_rerere(r, opts->allow_rerere_auto);
 		goto leave;
@@ -3172,7 +3171,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 		item->offset_in_buf = todo_list->buf.len;
 		subject_len = find_commit_subject(commit_buffer, &subject);
 		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
-			short_commit_name(commit), subject_len, subject);
+			short_commit_name(the_repository, commit),
+			subject_len, subject);
 		repo_unuse_commit_buffer(the_repository, commit,
 					 commit_buffer);
 	}
@@ -3593,7 +3593,7 @@ static int error_with_patch(struct repository *r,
 	} else if (exit_code) {
 		if (commit)
 			fprintf_ln(stderr, _("Could not apply %s... %.*s"),
-				   short_commit_name(commit), subject_len, subject);
+				   short_commit_name(r, commit), subject_len, subject);
 		else
 			/*
 			 * We don't have the hash of the parent so
@@ -4728,7 +4728,7 @@ static int pick_commits(struct repository *r,
 						term_clear_line();
 					fprintf(stderr,
 						_("Stopped at %s...  %.*s\n"),
-						short_commit_name(commit),
+						short_commit_name(r, commit),
 						item->arg_len, arg);
 				}
 				return error_with_patch(r, commit,
@@ -5564,7 +5564,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		if (!is_empty && (commit->object.flags & PATCHSAME)) {
 			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
 				warning(_("skipped previously applied commit %s"),
-					short_commit_name(commit));
+					short_commit_name(the_repository, commit));
 			skipped_commit = 1;
 			continue;
 		}
@@ -5800,7 +5800,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 		if (!is_empty && (commit->object.flags & PATCHSAME)) {
 			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
 				warning(_("skipped previously applied commit %s"),
-					short_commit_name(commit));
+					short_commit_name(r, commit));
 			skipped_commit = 1;
 			continue;
 		}
@@ -5892,7 +5892,8 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,
 	todo_list->alloc = alloc;
 }
 
-static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_list,
+static void todo_list_to_strbuf(struct repository *r,
+				struct todo_list *todo_list,
 				struct strbuf *buf, int num, unsigned flags)
 {
 	struct todo_item *item;
@@ -5921,7 +5922,7 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
 		/* add commit id */
 		if (item->commit) {
 			const char *oid = flags & TODO_LIST_SHORTEN_IDS ?
-					  short_commit_name(item->commit) :
+					  short_commit_name(r, item->commit) :
 					  oid_to_hex(&item->commit->object.oid);
 
 			if (item->command == TODO_FIXUP) {
-- 
2.42.0.528.g7950723a09

Re: [PATCH v2 01/22] sequencer: use repository parameter in short_commit_name()

[Phillip Wood]

On 30/08/2023 00:43, Jeff King wrote:
> Instead of just using the_repository, we can take a repository parameter
> from the caller. Most of them already have one, and doing so clears up a
> few -Wunused-parameter warnings. There are still a few callers which use
> the_repository, but this pushes us one small step forward to eventually
> getting rid of those.
> 
> Note that a few of these functions have a "rev_info" whose "repo"
> parameter could probably be used instead of the_repository. I'm leaving
> that for further cleanups, as it's not immediately obvious that
> revs->repo is always valid, and there's quite a bit of other possible
> refactoring here (even getting rid of some "struct repository" arguments
> in favor of revs->repo).

I think opts->revs is only initialized when we're building to todo list 
for cherry-picking/reverting a sequence of commits so I the scope for 
removing "struct repository" arguments is pretty limited as any function 
that is called by "cherry-pick --continue" or rebase needs a separate 
repository argument. I'm pretty sure your v1 was safe but this version 
is much more obviously safe.

Best Wishes

Phillip

[PATCH v2 02/22] sequencer: mark repository argument as unused

[Jeff King]

In sequencer_get_last_command(), we don't ever look at the repository
parameter. This is due to ed5b1ca10b (status: do not report errors in
sequencer/todo, 2019-06-27), which dropped the call to parse_insn_line().

However, it _should_ be used when calling into git_path_* functions,
but the one we use here is declared with the non-REPO variant of
GIT_PATH_FUNC(), and so just uses the_repository internally.

We could change the path helper to use REPO_GIT_PATH_FUNC(), but doing
so piecemeal is not great. There are 41 uses of GIT_PATH_FUNC() in
sequencer.c, and inconsistently switching one makes the code more
confusing. Likewise, this one function is used in half a dozen other
spots, all of which would need to start passing in a repository argument
(with rippling effects up the call stack).

So let's punt on that for now and just silence any -Wunused-parameter
warning.

Note that we could also drop this parameter entirely, as the function is
always called directly, and not as a callback that has to conform to
some external interface. But since we'd eventually want to use the
repository parameter, let's leave it in place to avoid disrupting the
callers twice.

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index cd5264171a..70696d4a65 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2649,7 +2649,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 	return item->commit ? 0 : -1;
 }
 
-int sequencer_get_last_command(struct repository *r, enum replay_action *action)
+int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
 {
 	const char *todo_file, *bol;
 	struct strbuf buf = STRBUF_INIT;
-- 
2.42.0.528.g7950723a09

[PATCH v2 03/22] ref-filter: mark unused parameters in parser callbacks

[Jeff King]

These are similar to the cases annotated in 5fe9e1ce2f (ref-filter: mark
unused callback parameters, 2023-02-24), but were added after that
commit.

Note that the ahead/behind callback ignores its "atom" parameter, which
is a little unusual, since that struct usually stores the result. But in
this case, the data is stored centrally in ref_array->counts, since we
want to compute all ahead/behinds at once, not per ref.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1bfaf20fbf..88b021dd1d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -549,7 +549,8 @@ static int signature_atom_parser(struct ref_format *format UNUSED,
 	return 0;
 }
 
-static int trailers_atom_parser(struct ref_format *format, struct used_atom *atom,
+static int trailers_atom_parser(struct ref_format *format UNUSED,
+				struct used_atom *atom,
 				const char *arg, struct strbuf *err)
 {
 	atom->u.contents.trailer_opts.no_divider = 1;
@@ -819,7 +820,7 @@ static int if_atom_parser(struct ref_format *format UNUSED,
 	return 0;
 }
 
-static int rest_atom_parser(struct ref_format *format,
+static int rest_atom_parser(struct ref_format *format UNUSED,
 			    struct used_atom *atom UNUSED,
 			    const char *arg, struct strbuf *err)
 {
@@ -828,7 +829,8 @@ static int rest_atom_parser(struct ref_format *format,
 	return 0;
 }
 
-static int ahead_behind_atom_parser(struct ref_format *format, struct used_atom *atom,
+static int ahead_behind_atom_parser(struct ref_format *format,
+				    struct used_atom *atom UNUSED,
 				    const char *arg, struct strbuf *err)
 {
 	struct string_list_item *item;
-- 
2.42.0.528.g7950723a09

[PATCH v2 04/22] pack-bitmap: mark unused parameters in show_object callback

[Jeff King]

This is similar to the cases in c50dca2a18 (list-objects: mark unused
callback parameters, 2023-02-24), but was added after that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 pack-bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 6afc03d1e4..ca8319b87c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1101,8 +1101,9 @@ static void show_boundary_commit(struct commit *commit, void *_data)
 	}
 }
 
-static void show_boundary_object(struct object *object,
-				 const char *name, void *data)
+static void show_boundary_object(struct object *object UNUSED,
+				 const char *name UNUSED,
+				 void *data UNUSED)
 {
 	BUG("should not be called");
 }
-- 
2.42.0.528.g7950723a09

[PATCH v2 05/22] worktree: mark unused parameters in each_ref_fn callback

[Jeff King]

This is similar to the cases in 63e14ee2d6 (refs: mark unused
each_ref_fn parameters, 2022-08-19), but it was added after that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 10db70b7ec..62b7e26f4b 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -628,10 +628,10 @@ static void print_preparing_worktree_line(int detach,
  *
  * Returns 0 on failure and non-zero on success.
  */
-static int first_valid_ref(const char *refname,
-			   const struct object_id *oid,
-			   int flags,
-			   void *cb_data)
+static int first_valid_ref(const char *refname UNUSED,
+			   const struct object_id *oid UNUSED,
+			   int flags UNUSED,
+			   void *cb_data UNUSED)
 {
 	return 1;
 }
-- 
2.42.0.528.g7950723a09

[PATCH v2 06/22] commit-graph: mark unused data parameters in generation callbacks

[Jeff King]

The compute_generation_info code uses function pointers to abstract the
get/set generation operations. Some callers don't need the extra void
data pointer, which should be annotated to appease -Wunused-parameter.

Note that we can drop the assignment of the "data" parameter in
compute_generation_numbers(), as we've just shown that neither of the
callbacks it uses will access it. This matches the caller in
ensure_generations_valid(), which already does not bother to set "data".

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 0aa1640d15..e11f326097 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1568,12 +1568,14 @@ static void compute_topological_levels(struct write_commit_graph_context *ctx)
 	stop_progress(&ctx->progress);
 }
 
-static timestamp_t get_generation_from_graph_data(struct commit *c, void *data)
+static timestamp_t get_generation_from_graph_data(struct commit *c,
+						  void *data UNUSED)
 {
 	return commit_graph_data_at(c)->generation;
 }
 
-static void set_generation_v2(struct commit *c, timestamp_t t, void *data)
+static void set_generation_v2(struct commit *c, timestamp_t t,
+			      void *data UNUSED)
 {
 	struct commit_graph_data *g = commit_graph_data_at(c);
 	g->generation = t;
@@ -1587,7 +1589,6 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 		.commits = &ctx->commits,
 		.get_generation = get_generation_from_graph_data,
 		.set_generation = set_generation_v2,
-		.data = ctx,
 	};
 
 	if (ctx->report_progress)
@@ -1616,7 +1617,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)
 }
 
 static void set_generation_in_graph_data(struct commit *c, timestamp_t t,
-					 void *data)
+					 void *data UNUSED)
 {
 	commit_graph_data_at(c)->generation = t;
 }
-- 
2.42.0.528.g7950723a09

[PATCH v2 07/22] ls-tree: mark unused parameter in callback

[Jeff King]

The formatting functions are dispatched from a table of function
pointers. The "path name only" function unsurprisingly does not need to
look at its "oid" parameter, but we must mark it as unused to make
-Wunused-parameter happy.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/ls-tree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index f558db5f3b..209d2dc0d5 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -241,7 +241,8 @@ static int show_tree_long(const struct object_id *oid, struct strbuf *base,
 	return recurse;
 }
 
-static int show_tree_name_only(const struct object_id *oid, struct strbuf *base,
+static int show_tree_name_only(const struct object_id *oid UNUSED,
+			       struct strbuf *base,
 			       const char *pathname, unsigned mode,
 			       void *context)
 {
-- 
2.42.0.528.g7950723a09

[PATCH v2 08/22] stash: mark unused parameter in diff callback

[Jeff King]

This is similar to the cases in 61bdc7c5d8 (diff: mark unused parameters
in callbacks, 2022-12-13), but I missed it when making that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/stash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index fe64cde9ce..c9365bea13 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -362,7 +362,7 @@ static int is_path_a_directory(const char *path)
 }
 
 static void add_diff_to_buf(struct diff_queue_struct *q,
-			    struct diff_options *options,
+			    struct diff_options *options UNUSED,
 			    void *data)
 {
 	int i;
-- 
2.42.0.528.g7950723a09

[PATCH v2 09/22] trace2: mark unused us_elapsed_absolute parameters

[Jeff King]

Many trace2 targets ignore the absolute elapsed time parameters.
However, the virtual interface needs to retain the parameter since it is
used by others (e.g., the perf target).

Signed-off-by: Jeff King <peff@peff.net>
---
 trace2/tr2_tgt_event.c  | 23 +++++++++++++----------
 trace2/tr2_tgt_normal.c | 20 ++++++++++++--------
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 53091781ec..59910a1a4f 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -335,7 +335,7 @@ static void fn_alias_fl(const char *file, int line, const char *alias,
 }
 
 static void fn_child_start_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute,
+			      uint64_t us_elapsed_absolute UNUSED,
 			      const struct child_process *cmd)
 {
 	const char *event_name = "child_start";
@@ -367,7 +367,8 @@ static void fn_child_start_fl(const char *file, int line,
 }
 
 static void fn_child_exit_fl(const char *file, int line,
-			     uint64_t us_elapsed_absolute, int cid, int pid,
+			     uint64_t us_elapsed_absolute UNUSED,
+			     int cid, int pid,
 			     int code, uint64_t us_elapsed_child)
 {
 	const char *event_name = "child_exit";
@@ -388,7 +389,8 @@ static void fn_child_exit_fl(const char *file, int line,
 }
 
 static void fn_child_ready_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int cid, int pid,
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int cid, int pid,
 			      const char *ready, uint64_t us_elapsed_child)
 {
 	const char *event_name = "child_ready";
@@ -409,7 +411,7 @@ static void fn_child_ready_fl(const char *file, int line,
 }
 
 static void fn_thread_start_fl(const char *file, int line,
-			       uint64_t us_elapsed_absolute)
+			       uint64_t us_elapsed_absolute UNUSED)
 {
 	const char *event_name = "thread_start";
 	struct json_writer jw = JSON_WRITER_INIT;
@@ -423,7 +425,7 @@ static void fn_thread_start_fl(const char *file, int line,
 }
 
 static void fn_thread_exit_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute,
+			      uint64_t us_elapsed_absolute UNUSED,
 			      uint64_t us_elapsed_thread)
 {
 	const char *event_name = "thread_exit";
@@ -439,7 +441,8 @@ static void fn_thread_exit_fl(const char *file, int line,
 	jw_release(&jw);
 }
 
-static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
+static void fn_exec_fl(const char *file, int line,
+		       uint64_t us_elapsed_absolute UNUSED,
 		       int exec_id, const char *exe, const char **argv)
 {
 	const char *event_name = "exec";
@@ -460,8 +463,8 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 }
 
 static void fn_exec_result_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int exec_id,
-			      int code)
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int exec_id, int code)
 {
 	const char *event_name = "exec_result";
 	struct json_writer jw = JSON_WRITER_INIT;
@@ -511,7 +514,7 @@ static void fn_repo_fl(const char *file, int line,
 }
 
 static void fn_region_enter_printf_va_fl(const char *file, int line,
-					 uint64_t us_elapsed_absolute,
+					 uint64_t us_elapsed_absolute UNUSED,
 					 const char *category,
 					 const char *label,
 					 const struct repository *repo,
@@ -538,7 +541,7 @@ static void fn_region_enter_printf_va_fl(const char *file, int line,
 }
 
 static void fn_region_leave_printf_va_fl(
-	const char *file, int line, uint64_t us_elapsed_absolute,
+	const char *file, int line, uint64_t us_elapsed_absolute UNUSED,
 	uint64_t us_elapsed_region, const char *category, const char *label,
 	const struct repository *repo, const char *fmt, va_list ap)
 {
diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index d25ea13164..38d5ebddf6 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -86,7 +86,7 @@ static void fn_version_fl(const char *file, int line)
 }
 
 static void fn_start_fl(const char *file, int line,
-			uint64_t us_elapsed_absolute, const char **argv)
+			uint64_t us_elapsed_absolute UNUSED, const char **argv)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
 
@@ -215,7 +215,7 @@ static void fn_alias_fl(const char *file, int line, const char *alias,
 }
 
 static void fn_child_start_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute,
+			      uint64_t us_elapsed_absolute UNUSED,
 			      const struct child_process *cmd)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -243,7 +243,8 @@ static void fn_child_start_fl(const char *file, int line,
 }
 
 static void fn_child_exit_fl(const char *file, int line,
-			     uint64_t us_elapsed_absolute, int cid, int pid,
+			     uint64_t us_elapsed_absolute UNUSED,
+			     int cid, int pid,
 			     int code, uint64_t us_elapsed_child)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -256,7 +257,8 @@ static void fn_child_exit_fl(const char *file, int line,
 }
 
 static void fn_child_ready_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int cid, int pid,
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int cid, int pid,
 			      const char *ready, uint64_t us_elapsed_child)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -268,7 +270,8 @@ static void fn_child_ready_fl(const char *file, int line,
 	strbuf_release(&buf_payload);
 }
 
-static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
+static void fn_exec_fl(const char *file, int line,
+		       uint64_t us_elapsed_absolute UNUSED,
 		       int exec_id, const char *exe, const char **argv)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
@@ -284,8 +287,8 @@ static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 }
 
 static void fn_exec_result_fl(const char *file, int line,
-			      uint64_t us_elapsed_absolute, int exec_id,
-			      int code)
+			      uint64_t us_elapsed_absolute UNUSED,
+			      int exec_id, int code)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
 
@@ -321,7 +324,8 @@ static void fn_repo_fl(const char *file, int line,
 }
 
 static void fn_printf_va_fl(const char *file, int line,
-			    uint64_t us_elapsed_absolute, const char *fmt,
+			    uint64_t us_elapsed_absolute UNUSED,
+			    const char *fmt,
 			    va_list ap)
 {
 	struct strbuf buf_payload = STRBUF_INIT;
-- 
2.42.0.528.g7950723a09

[PATCH v2 10/22] trace2: mark unused config callback parameter

[Jeff King]

This should have been part of 783a86c142 (config: mark unused callback
parameters, 2022-08-19), but was missed in that commit.

Signed-off-by: Jeff King <peff@peff.net>
---
 trace2/tr2_sysenv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/trace2/tr2_sysenv.c b/trace2/tr2_sysenv.c
index f26ec95ab4..d3ecac2772 100644
--- a/trace2/tr2_sysenv.c
+++ b/trace2/tr2_sysenv.c
@@ -58,7 +58,8 @@ static struct tr2_sysenv_entry tr2_sysenv_settings[] = {
 /* clang-format on */
 
 static int tr2_sysenv_cb(const char *key, const char *value,
-			 const struct config_context *ctx UNUSED, void *d)
+			 const struct config_context *ctx UNUSED,
+			 void *d UNUSED)
 {
 	int k;
 
-- 
2.42.0.528.g7950723a09

[PATCH v2 11/22] test-trace2: mark unused argv/argc parameters

[Jeff King]

The trace2 test helper uses function pointers to dispatch to individual
tests. Not all tests bother looking at their argv/argc parameters. We
could tighten this up (e.g., complaining when seeing unexpected
parameters), but for internal test code it's not worth worrying about.

This is similar in spirit to 126e3b3d2a (t/helper: mark unused argv/argc
arguments, 2023-03-28).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-trace2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index 20c7495f38..d5ca0046c8 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -45,7 +45,7 @@ static int get_i(int *p_value, const char *data)
  * [] "def_param" events for all of the "interesting" pre-defined
  * config settings.
  */
-static int ut_001return(int argc, const char **argv)
+static int ut_001return(int argc UNUSED, const char **argv)
 {
 	int rc;
 
@@ -65,7 +65,7 @@ static int ut_001return(int argc, const char **argv)
  * [] "def_param" events for all of the "interesting" pre-defined
  * config settings.
  */
-static int ut_002exit(int argc, const char **argv)
+static int ut_002exit(int argc UNUSED, const char **argv)
 {
 	int rc;
 
@@ -201,7 +201,7 @@ static int ut_006data(int argc, const char **argv)
 	return 0;
 }
 
-static int ut_007BUG(int argc, const char **argv)
+static int ut_007BUG(int argc UNUSED, const char **argv UNUSED)
 {
 	/*
 	 * Exercise BUG() to ensure that the message is printed to trace2.
-- 
2.42.0.528.g7950723a09

[PATCH v2 12/22] grep: mark unused parameter in output function

[Jeff King]

This is a callback used with grep_options.output, but we don't look at
the grep_opt parameter, as we're just writing the output to stdout.

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index 0904d55b24..0124eb1960 100644
--- a/grep.c
+++ b/grep.c
@@ -17,7 +17,7 @@ static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs,
 				 struct index_state *istate);
 
-static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+static void std_output(struct grep_opt *opt UNUSED, const void *buf, size_t size)
 {
 	fwrite(buf, size, 1, stdout);
 }
-- 
2.42.0.528.g7950723a09

[PATCH v2 13/22] add-interactive: mark unused callback parameters

[Jeff King]

The interactive commands are dispatched from a table of abstract
pointers, but not every command uses every parameter it receives. Mark
the unused ones to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 add-interactive.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index add9a1ad43..852e8e6b2f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1021,9 +1021,9 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
 	return res;
 }
 
-static int run_help(struct add_i_state *s, const struct pathspec *unused_ps,
-		    struct prefix_item_list *unused_files,
-		    struct list_and_choose_options *unused_opts)
+static int run_help(struct add_i_state *s, const struct pathspec *ps UNUSED,
+		    struct prefix_item_list *files UNUSED,
+		    struct list_and_choose_options *opts UNUSED)
 {
 	color_fprintf_ln(stdout, s->help_color, "status        - %s",
 			 _("show paths with changes"));
@@ -1074,7 +1074,7 @@ struct print_command_item_data {
 	const char *color, *reset;
 };
 
-static void print_command_item(int i, int selected,
+static void print_command_item(int i, int selected UNUSED,
 			       struct string_list_item *item,
 			       void *print_command_item_data)
 {
-- 
2.42.0.528.g7950723a09

[PATCH v2 14/22] negotiator/noop: mark unused callback parameters

[Jeff King]

The noop negotiator unsurprisingly does not bother looking at any of its
parameters. Mark them unused to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 negotiator/noop.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/negotiator/noop.c b/negotiator/noop.c
index 7b72937686..de39028ab7 100644
--- a/negotiator/noop.c
+++ b/negotiator/noop.c
@@ -3,22 +3,24 @@
 #include "../commit.h"
 #include "../fetch-negotiator.h"
 
-static void known_common(struct fetch_negotiator *n, struct commit *c)
+static void known_common(struct fetch_negotiator *n UNUSED,
+			 struct commit *c UNUSED)
 {
 	/* do nothing */
 }
 
-static void add_tip(struct fetch_negotiator *n, struct commit *c)
+static void add_tip(struct fetch_negotiator *n UNUSED,
+		    struct commit *c UNUSED)
 {
 	/* do nothing */
 }
 
-static const struct object_id *next(struct fetch_negotiator *n)
+static const struct object_id *next(struct fetch_negotiator *n UNUSED)
 {
 	return NULL;
 }
 
-static int ack(struct fetch_negotiator *n, struct commit *c)
+static int ack(struct fetch_negotiator *n UNUSED, struct commit *c UNUSED)
 {
 	/*
 	 * This negotiator does not emit any commits, so there is no commit to
@@ -28,7 +30,7 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
 	return 0;
 }
 
-static void release(struct fetch_negotiator *n)
+static void release(struct fetch_negotiator *n UNUSED)
 {
 	/* nothing to release */
 }
-- 
2.42.0.528.g7950723a09

[PATCH v2 15/22] worktree: mark unused parameters in noop repair callback

[Jeff King]

The noop repair callback unsurprisingly does not look at any of its
parameters. Mark them as unused to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 worktree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/worktree.c b/worktree.c
index b8cf29e6a1..a56a6c2a3d 100644
--- a/worktree.c
+++ b/worktree.c
@@ -581,8 +581,10 @@ static void repair_gitfile(struct worktree *wt,
 	strbuf_release(&dotgit);
 }
 
-static void repair_noop(int iserr, const char *path, const char *msg,
-			void *cb_data)
+static void repair_noop(int iserr UNUSED,
+			const char *path UNUSED,
+			const char *msg UNUSED,
+			void *cb_data UNUSED)
 {
 	/* nothing */
 }
-- 
2.42.0.528.g7950723a09

[PATCH v2 16/22] imap-send: mark unused parameters with NO_OPENSSL

[Jeff King]

Earlier patches annotating unused parameters in imap-send missed a few
cases in code that is compiled only with NO_OPENSSL. These need to
retain the extra parameters to match the interfaces used when we compile
with openssl support.

Note in the case of socket_perror() that the function declaration and
parts of its code are shared between the two cases, and only the openssl
code looks at "sock". So we can't simply mark the parameter as always
unused. Instead, we can add a noop statement that references it. This is
ugly, but should be portable.

Signed-off-by: Jeff King <peff@peff.net>
---
 imap-send.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 06386e0b3b..996651e4f8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -206,10 +206,14 @@ static void socket_perror(const char *func, struct imap_socket *sock, int ret)
 		else
 			fprintf(stderr, "%s: unexpected EOF\n", func);
 	}
+	/* mark as used to appease -Wunused-parameter with NO_OPENSSL */
+	(void)sock;
 }
 
 #ifdef NO_OPENSSL
-static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int verify)
+static int ssl_socket_connect(struct imap_socket *sock UNUSED,
+			      int use_tls_only UNUSED,
+			      int verify UNUSED)
 {
 	fprintf(stderr, "SSL requested but SSL support not compiled in\n");
 	return -1;
@@ -904,7 +908,9 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
 
 #else
 
-static char *cram(const char *challenge_64, const char *user, const char *pass)
+static char *cram(const char *challenge_64 UNUSED,
+		  const char *user UNUSED,
+		  const char *pass UNUSED)
 {
 	die("If you want to use CRAM-MD5 authenticate method, "
 	    "you have to build git-imap-send with OpenSSL library.");
-- 
2.42.0.528.g7950723a09

[PATCH v2 17/22] grep: mark unused parmaeters in pcre fallbacks

[Jeff King]

When USE_LIBPCRE2 is not defined, we compile several noop fallbacks.
These need to have their parameters annotated to avoid
-Wunused-parameter warnings (and obviously we cannot remove the
parameters, since the functions must match the non-fallback versions).

Signed-off-by: Jeff King <peff@peff.net>
---
 grep.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 0124eb1960..fc2d0c837a 100644
--- a/grep.c
+++ b/grep.c
@@ -452,18 +452,20 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_general_context_free(p->pcre2_general_context);
 }
 #else /* !USE_LIBPCRE2 */
-static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
+static void compile_pcre2_pattern(struct grep_pat *p UNUSED,
+				  const struct grep_opt *opt UNUSED)
 {
 	die("cannot use Perl-compatible regexes when not compiled with USE_LIBPCRE");
 }
 
-static int pcre2match(struct grep_pat *p, const char *line, const char *eol,
-		regmatch_t *match, int eflags)
+static int pcre2match(struct grep_pat *p UNUSED, const char *line UNUSED,
+		      const char *eol UNUSED, regmatch_t *match UNUSED,
+		      int eflags UNUSED)
 {
 	return 1;
 }
 
-static void free_pcre2_pattern(struct grep_pat *p)
+static void free_pcre2_pattern(struct grep_pat *p UNUSED)
 {
 }
 
-- 
2.42.0.528.g7950723a09

[PATCH v2 18/22] credential: mark unused parameter in urlmatch callback

[Jeff King]

Our select_all() callback does not need to actually look at its
parameters, since the point is to match everything. But we need to mark
its parameters to satisfy -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/credential.c b/credential.c
index d664754163..18098bd35e 100644
--- a/credential.c
+++ b/credential.c
@@ -88,8 +88,8 @@ static int proto_is_http(const char *s)
 static void credential_describe(struct credential *c, struct strbuf *out);
 static void credential_format(struct credential *c, struct strbuf *out);
 
-static int select_all(const struct urlmatch_item *a,
-		      const struct urlmatch_item *b)
+static int select_all(const struct urlmatch_item *a UNUSED,
+		      const struct urlmatch_item *b UNUSED)
 {
 	return 0;
 }
-- 
2.42.0.528.g7950723a09

[PATCH v2 19/22] fetch: mark unused parameter in ref_transaction callback

[Jeff King]

Since this callback is just trying to collect the set of queued tag
updates, there is no need for it to look at old_oid at all. Mark it as
unused to appease -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index eed4a7cdb6..8f93529505 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -308,7 +308,7 @@ static void clear_item(struct refname_hash_entry *item)
 
 
 static void add_already_queued_tags(const char *refname,
-				    const struct object_id *old_oid,
+				    const struct object_id *old_oid UNUSED,
 				    const struct object_id *new_oid,
 				    void *cb_data)
 {
-- 
2.42.0.528.g7950723a09

[PATCH v2 20/22] bundle-uri: mark unused parameters in callbacks

[Jeff King]

The first hunk is similar to 02c3c59e62 (hashmap: mark unused callback
parameters, 2022-08-19), but was added after that commit.

The other two are used with for_all_bundles_in_list(), but don't use
their void data pointer.

Signed-off-by: Jeff King <peff@peff.net>
---
 bundle-uri.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bundle-uri.c b/bundle-uri.c
index 4b5c49b93d..8492fffd2f 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -20,7 +20,7 @@ static struct {
 	{ BUNDLE_HEURISTIC_CREATIONTOKEN, "creationToken" },
 };
 
-static int compare_bundles(const void *hashmap_cmp_fn_data,
+static int compare_bundles(const void *hashmap_cmp_fn_data UNUSED,
 			   const struct hashmap_entry *he1,
 			   const struct hashmap_entry *he2,
 			   const void *id)
@@ -45,7 +45,7 @@ void init_bundle_list(struct bundle_list *list)
 }
 
 static int clear_remote_bundle_info(struct remote_bundle_info *bundle,
-				    void *data)
+				    void *data UNUSED)
 {
 	FREE_AND_NULL(bundle->id);
 	FREE_AND_NULL(bundle->uri);
@@ -779,7 +779,7 @@ static int unbundle_all_bundles(struct repository *r,
 	return 0;
 }
 
-static int unlink_bundle(struct remote_bundle_info *info, void *data)
+static int unlink_bundle(struct remote_bundle_info *info, void *data UNUSED)
 {
 	if (info->file)
 		unlink_or_warn(info->file);
-- 
2.42.0.528.g7950723a09

[PATCH v2 21/22] gc: mark unused descriptors in scheduler callbacks

[Jeff King]

Each of the scheduler update callbacks gets the descriptor of the lock
file, but only the crontab updater needs it. We have to retain the
unused descriptors because these are dispatched from a table of function
pointers, but we should mark them to silence -Wunused-parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/gc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 1f53b66c7b..369bd43fb2 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1934,7 +1934,7 @@ static int launchctl_add_plists(void)
 	       launchctl_schedule_plist(exec_path, SCHEDULE_WEEKLY);
 }
 
-static int launchctl_update_schedule(int run_maintenance, int fd)
+static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)
 {
 	if (run_maintenance)
 		return launchctl_add_plists();
@@ -2115,7 +2115,7 @@ static int schtasks_schedule_tasks(void)
 	       schtasks_schedule_task(exec_path, SCHEDULE_WEEKLY);
 }
 
-static int schtasks_update_schedule(int run_maintenance, int fd)
+static int schtasks_update_schedule(int run_maintenance, int fd UNUSED)
 {
 	if (run_maintenance)
 		return schtasks_schedule_tasks();
@@ -2556,7 +2556,7 @@ static int systemd_timer_setup_units(void)
 	return ret;
 }
 
-static int systemd_timer_update_schedule(int run_maintenance, int fd)
+static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
 {
 	if (run_maintenance)
 		return systemd_timer_setup_units();
-- 
2.42.0.528.g7950723a09

[PATCH v2 22/22] update-ref: mark unused parameter in parser callbacks

[Jeff King]

The parsing of stdin is driven by a table of function pointers; mark
unused parameters in concrete functions to avoid -Wunused-parameter
warnings.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/update-ref.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 242102273e..c0c4e65e6f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -311,8 +311,8 @@ static void report_ok(const char *command)
 	fflush(stdout);
 }
 
-static void parse_cmd_option(struct ref_transaction *transaction,
-			     const char *next, const char *end)
+static void parse_cmd_option(struct ref_transaction *transaction UNUSED,
+			     const char *next, const char *end UNUSED)
 {
 	const char *rest;
 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
@@ -321,16 +321,16 @@ static void parse_cmd_option(struct ref_transaction *transaction,
 		die("option unknown: %s", next);
 }
 
-static void parse_cmd_start(struct ref_transaction *transaction,
-			    const char *next, const char *end)
+static void parse_cmd_start(struct ref_transaction *transaction UNUSED,
+			    const char *next, const char *end UNUSED)
 {
 	if (*next != line_termination)
 		die("start: extra input: %s", next);
 	report_ok("start");
 }
 
 static void parse_cmd_prepare(struct ref_transaction *transaction,
-			      const char *next, const char *end)
+			      const char *next, const char *end UNUSED)
 {
 	struct strbuf error = STRBUF_INIT;
 	if (*next != line_termination)
@@ -341,7 +341,7 @@ static void parse_cmd_prepare(struct ref_transaction *transaction,
 }
 
 static void parse_cmd_abort(struct ref_transaction *transaction,
-			    const char *next, const char *end)
+			    const char *next, const char *end UNUSED)
 {
 	struct strbuf error = STRBUF_INIT;
 	if (*next != line_termination)
@@ -352,7 +352,7 @@ static void parse_cmd_abort(struct ref_transaction *transaction,
 }
 
 static void parse_cmd_commit(struct ref_transaction *transaction,
-			     const char *next, const char *end)
+			     const char *next, const char *end UNUSED)
 {
 	struct strbuf error = STRBUF_INIT;
 	if (*next != line_termination)
-- 
2.42.0.528.g7950723a09

Re: [PATCH v2 0/22] Yet Another Unused Parameter Series

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 28, 2023 at 05:46:04PM -0400, Jeff King wrote:
>
>> I'm back with another exciting installment of -Wunused-parameter warning
>> fixes. Most of these are pretty boring and obvious; the first two are
>> the most interesting in terms of rationale.
>> 
>> I promise we're closing in on the finish line here. I only have about 20
>> patches left after this, at which point we should be able to turn on the
>> warning by default for developer builds.
>
> And here's a v2 based on feedback from round 1. The changes are small,
> so I'll let the range-diff below speak for itself.

The first one in this round is consistently more conservative
compared to the previous round, which is very nice.

All other updates do look good, too.

Will replace.  Thanks.