[PATCH 0/14] "final" batch of unused parameter cleanups

[PATCH 0/14] "final" batch of unused parameter cleanups

[Jeff King]

This is a continuation of my efforts to get us compiling with
-Wunused-parameter.  This round finishes most of the cleanups and fixes
I intend to do (though I have a handful of cleanup cases that I'm still
poking at to make sure they're not in fact bugs). After that, I have
some patches to annotate unused parameters we can't get rid of (e.g.,
for cases where we're conforming to a callback interface), and then we
can finally flip the warning on for developer-mode.

This series has two small conflicts with what's in "pu":

  - there's a minor textual conflict in upload-pack

  - it changes cmd_rebase__interactive(), which moved into
    builtin/rebase.c in pw/rebase-i-internal. Because the old file went
    away, this results in a delete/modified conflict, and you have to
    manually fix up the result, like so:

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
deleted file mode 100644
index 72fd4b53a8..0000000000
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8c2cbc72f6..cc1e1a997d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -508,7 +508,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	if (argc == 1)
 		usage_with_options(builtin_rebase_interactive_usage, options);
 
-	argc = parse_options(argc, argv, NULL, options,
+	argc = parse_options(argc, argv, prefix, options,
 			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
 
 	if (!is_null_oid(&squash_onto))

Most of the patches are pretty straightforward and won't have any
behavior change (the thing to review in them is confirming that not
using the parameter is the right thing to do; I've tried to make a case
for each in the commit message). The trickiest one is patch 14.

  [01/14]: cmd_{read,write}_tree: rename "unused" variable that is used
  [02/14]: submodule: drop unused prefix parameter from some functions
  [03/14]: builtin: consistently pass cmd_* prefix to parse_options
  [04/14]: clone: drop dest parameter from copy_alternates()
  [05/14]: read-cache: drop unused parameter from threaded load
  [06/14]: wt-status: drop unused status parameter
  [07/14]: mktree: drop unused length parameter
  [08/14]: name-rev: drop unused parameters from is_better_name()
  [09/14]: pack-objects: drop unused rev_info parameters
  [10/14]: receive-pack: drop unused "commands" from prepare_shallow_update()
  [11/14]: remove_all_fetch_refspecs(): drop unused "remote" parameter
  [12/14]: rev-list: drop unused void pointer from finish_commit()
  [13/14]: show-branch: drop unused parameter from show_independent()
  [14/14]: verify-commit: simplify parameters to run_gpg_verify()

 builtin/clone.c               |  5 ++---
 builtin/column.c              |  2 +-
 builtin/hash-object.c         |  2 +-
 builtin/mktree.c              |  4 ++--
 builtin/name-rev.c            |  5 +----
 builtin/pack-objects.c        |  8 ++++----
 builtin/range-diff.c          |  2 +-
 builtin/read-tree.c           |  4 ++--
 builtin/rebase--interactive.c |  2 +-
 builtin/receive-pack.c        |  5 ++---
 builtin/remote.c              |  4 ++--
 builtin/rev-list.c            | 10 +++++-----
 builtin/rm.c                  |  6 +++---
 builtin/show-branch.c         |  3 +--
 builtin/submodule--helper.c   |  3 +--
 builtin/upload-pack.c         |  2 +-
 builtin/verify-commit.c       | 23 ++++++++---------------
 builtin/write-tree.c          | 12 ++++++------
 read-cache.c                  |  4 ++--
 submodule.c                   | 10 ++++------
 submodule.h                   |  3 +--
 wt-status.c                   |  6 ++----
 22 files changed, 53 insertions(+), 72 deletions(-)

[PATCH 01/14] cmd_{read,write}_tree: rename "unused" variable that is used

[Jeff King]

The "prefix" variable passed by git.c into the builtin cmd_read_tree()
and cmd_write_tree() functions is named "unused_prefix". But we do in
fact pass it to parse_options(), which may use the prefix to adjust any
filename options. Let's get rid of this confusing name.

However, we can't just call it "prefix". The reason these variables were
renamed in the first place is that they shadowed local variables named
"prefix", because these commands both take a "--prefix" option.

So let's rename the parameters, but try to reduce further confusion:

  1. In both cases we'll call them "cmd_prefix" to mark that they're
     part of the cmd_* interface.

  2. In cmd_write_tree(), we'll rename the local prefix variable to
     "tree_prefix" to make it more clear that we're talking about the
     prefix to be used for the tree we're writing.

  3. In cmd_read_tree(), the "prefix" local has since migrated into
     "struct unpack_trees_options". We'll leave that alone, as the
     context within the struct makes its meaning clear (we actually
     _could_ just call the parameter "prefix" now, but that invites
     confusion in the other direction).

Signed-off-by: Jeff King <peff@peff.net>
---
I kind of hate "cmd_prefix"; I was tempted to just call it "prefix" so
that all of the cmd_* functions were consistent, but I worried that it
really would get confused with the local variables (even if those
variables are renamed, as I do here).

 builtin/read-tree.c  |  4 ++--
 builtin/write-tree.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 5c9c082595..ca5e655d2f 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -111,7 +111,7 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
+int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
 {
 	int i, stage = 0;
 	struct object_id oid;
@@ -165,7 +165,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 
 	git_config(git_read_tree_config, NULL);
 
-	argc = parse_options(argc, argv, unused_prefix, read_tree_options,
+	argc = parse_options(argc, argv, cmd_prefix, read_tree_options,
 			     read_tree_usage, 0);
 
 	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 3d46d22ee5..45d61707e7 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -16,16 +16,16 @@ static const char * const write_tree_usage[] = {
 	NULL
 };
 
-int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
+int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
 {
 	int flags = 0, ret;
-	const char *prefix = NULL;
+	const char *tree_prefix = NULL;
 	struct object_id oid;
 	const char *me = "git-write-tree";
 	struct option write_tree_options[] = {
 		OPT_BIT(0, "missing-ok", &flags, N_("allow missing objects"),
 			WRITE_TREE_MISSING_OK),
-		OPT_STRING(0, "prefix", &prefix, N_("<prefix>/"),
+		OPT_STRING(0, "prefix", &tree_prefix, N_("<prefix>/"),
 			   N_("write tree object for a subdirectory <prefix>")),
 		{ OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL,
 		  N_("only useful for debugging"),
@@ -35,10 +35,10 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	};
 
 	git_config(git_default_config, NULL);
-	argc = parse_options(argc, argv, unused_prefix, write_tree_options,
+	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
 			     write_tree_usage, 0);
 
-	ret = write_cache_as_tree(&oid, flags, prefix);
+	ret = write_cache_as_tree(&oid, flags, tree_prefix);
 	switch (ret) {
 	case 0:
 		printf("%s\n", oid_to_hex(&oid));
@@ -50,7 +50,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 		die("%s: error building trees", me);
 		break;
 	case WRITE_TREE_PREFIX_ERROR:
-		die("%s: prefix %s not found", me, prefix);
+		die("%s: prefix %s not found", me, tree_prefix);
 		break;
 	}
 	return ret;
-- 
2.21.0.1382.g4c6032d436

[PATCH 02/14] submodule: drop unused prefix parameter from some functions

[Jeff King]

We stopped using the "prefix" parameter of
relocate_single_git_dir_into_superproject() and its callers in
202275b96b (submodule.c: get_super_prefix_or_empty, 2017-03-14), where
we switched to using the environment global directly.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rm.c                |  6 +++---
 builtin/submodule--helper.c |  3 +--
 submodule.c                 | 10 ++++------
 submodule.h                 |  3 +--
 4 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 90cbe896c9..be8edc6d1e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -61,7 +61,7 @@ static void print_error_files(struct string_list *files_list,
 	}
 }
 
-static void submodules_absorb_gitdir_if_needed(const char *prefix)
+static void submodules_absorb_gitdir_if_needed(void)
 {
 	int i;
 	for (i = 0; i < list.nr; i++) {
@@ -83,7 +83,7 @@ static void submodules_absorb_gitdir_if_needed(const char *prefix)
 			continue;
 
 		if (!submodule_uses_gitfile(name))
-			absorb_git_dir_into_superproject(prefix, name,
+			absorb_git_dir_into_superproject(name,
 				ABSORB_GITDIR_RECURSE_SUBMODULES);
 	}
 }
@@ -313,7 +313,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!index_only)
-		submodules_absorb_gitdir_if_needed(prefix);
+		submodules_absorb_gitdir_if_needed();
 
 	/*
 	 * If not forced, the file, the index and the HEAD (if exists)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8c72ea864c..7262f1a000 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2107,8 +2107,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
 		return 1;
 
 	for (i = 0; i < list.nr; i++)
-		absorb_git_dir_into_superproject(prefix,
-				list.entries[i]->name, flags);
+		absorb_git_dir_into_superproject(list.entries[i]->name, flags);
 
 	return 0;
 }
diff --git a/submodule.c b/submodule.c
index 2cfaba0599..0f199c5137 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1910,7 +1910,7 @@ int submodule_move_head(const char *path,
 	if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
 		if (old_head) {
 			if (!submodule_uses_gitfile(path))
-				absorb_git_dir_into_superproject("", path,
+				absorb_git_dir_into_superproject(path,
 					ABSORB_GITDIR_RECURSE_SUBMODULES);
 		} else {
 			char *gitdir = xstrfmt("%s/modules/%s",
@@ -1997,8 +1997,7 @@ int submodule_move_head(const char *path,
  * Embeds a single submodules git directory into the superprojects git dir,
  * non recursively.
  */
-static void relocate_single_git_dir_into_superproject(const char *prefix,
-						      const char *path)
+static void relocate_single_git_dir_into_superproject(const char *path)
 {
 	char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
 	const char *new_git_dir;
@@ -2040,8 +2039,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
  * having its git directory within the working tree to the git dir nested
  * in its superprojects git dir under modules/.
  */
-void absorb_git_dir_into_superproject(const char *prefix,
-				      const char *path,
+void absorb_git_dir_into_superproject(const char *path,
 				      unsigned flags)
 {
 	int err_code;
@@ -2082,7 +2080,7 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		char *real_common_git_dir = real_pathdup(get_git_common_dir(), 1);
 
 		if (!starts_with(real_sub_git_dir, real_common_git_dir))
-			relocate_single_git_dir_into_superproject(prefix, path);
+			relocate_single_git_dir_into_superproject(path);
 
 		free(real_sub_git_dir);
 		free(real_common_git_dir);
diff --git a/submodule.h b/submodule.h
index 9e18e9b807..8072e6d6dd 100644
--- a/submodule.h
+++ b/submodule.h
@@ -141,8 +141,7 @@ void submodule_unset_core_worktree(const struct submodule *sub);
 void prepare_submodule_repo_env(struct argv_array *out);
 
 #define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
-void absorb_git_dir_into_superproject(const char *prefix,
-				      const char *path,
+void absorb_git_dir_into_superproject(const char *path,
 				      unsigned flags);
 
 /*
-- 
2.21.0.1382.g4c6032d436

[PATCH 03/14] builtin: consistently pass cmd_* prefix to parse_options

[Jeff King]

If a builtin uses RUN_SETUP to request that git.c enter the repository
directory, we'll get passed in a "prefix" variable with the path to the
original directory.  It's important to pass this to parse_options(),
since we may use it to fix up relative OPT_FILENAME() options. Some
builtins don't bother; let's make sure we do so consistently.

There may not be any particular bugs fixed here; OPT_FILENAME is
actually pretty rare, and none of these commands use it directly.
However, this does future-proof us against somebody adding an option
that uses it and creating a subtle bug that only shows up when you're in
a subdirectory of the repository.

In some cases, like hash-object and upload-pack, we don't specify
RUN_SETUP, so we know the prefix will always be empty. It's still worth
passing the variable along to keep the idiom consistent across all
builtins (and of course it protects us if they ever _did_ switch to
using RUN_SETUP).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/column.c              | 2 +-
 builtin/hash-object.c         | 2 +-
 builtin/range-diff.c          | 2 +-
 builtin/rebase--interactive.c | 2 +-
 builtin/upload-pack.c         | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/column.c b/builtin/column.c
index 5228ccf37a..e815e148aa 100644
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -43,7 +43,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)
 
 	memset(&copts, 0, sizeof(copts));
 	copts.padding = 1;
-	argc = parse_options(argc, argv, "", options, builtin_column_usage, 0);
+	argc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);
 	if (argc)
 		usage_with_options(builtin_column_usage, options);
 	if (real_command || command) {
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index e055c11103..640ef4ded5 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -108,7 +108,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)
 	int i;
 	const char *errstr = NULL;
 
-	argc = parse_options(argc, argv, NULL, hash_object_options,
+	argc = parse_options(argc, argv, prefix, hash_object_options,
 			     hash_object_usage, 0);
 
 	if (flags & HASH_WRITE_OBJECT)
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 784bd19321..9202e75544 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -32,7 +32,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	repo_diff_setup(the_repository, &diffopt);
 
 	options = parse_options_concat(range_diff_options, diffopt.parseopts);
-	argc = parse_options(argc, argv, NULL, options,
+	argc = parse_options(argc, argv, prefix, options,
 			     builtin_range_diff_usage, 0);
 
 	diff_setup_done(&diffopt);
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 4535523bf5..72fd4b53a8 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -302,7 +302,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 	if (argc == 1)
 		usage_with_options(builtin_rebase_interactive_usage, options);
 
-	argc = parse_options(argc, argv, NULL, options,
+	argc = parse_options(argc, argv, prefix, options,
 			builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);
 
 	opts.gpg_sign = xstrdup_or_null(opts.gpg_sign);
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1..6da8fa2607 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -33,7 +33,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	packet_trace_identity("upload-pack");
 	read_replace_refs = 0;
 
-	argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
+	argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
 
 	if (argc != 1)
 		usage_with_options(upload_pack_usage, options);
-- 
2.21.0.1382.g4c6032d436

[PATCH 04/14] clone: drop dest parameter from copy_alternates()

[Jeff King]

Ever since the inception of this function in e6baf4a1ae (clone: clone
from a repository with relative alternates, 2011-08-22), the "dest"
parameter has been unused. Instead, we use add_to_alternates_file(),
which relies on git_pathdup() to find the right file. That in turn works
because we will have initialized and entered the destination repo by
this point.

It's a bit subtle, but this is how it has always worked. And if our
assumptions change, the test in t5601 from e6baf4a1ae should let us
know.

In the meantime, let's drop this unused and confusing parameter from
copy_alternates().

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

diff --git a/builtin/clone.c b/builtin/clone.c
index 31a47d190a..9ddf4f99e5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -357,8 +357,7 @@ static void setup_reference(void)
 			     add_one_reference, &required);
 }
 
-static void copy_alternates(struct strbuf *src, struct strbuf *dst,
-			    const char *src_repo)
+static void copy_alternates(struct strbuf *src, const char *src_repo)
 {
 	/*
 	 * Read from the source objects/info/alternates file
@@ -439,7 +438,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
 
 		/* Files that cannot be copied bit-for-bit... */
 		if (!strcmp(src->buf + src_baselen, "/info/alternates")) {
-			copy_alternates(src, dest, src_repo);
+			copy_alternates(src, src_repo);
 			continue;
 		}
 
-- 
2.21.0.1382.g4c6032d436

[PATCH 05/14] read-cache: drop unused parameter from threaded load

[Jeff King]

The load_cache_entries_threaded() function takes a src_offset parameter
that it doesn't use. This has been there since its inception in
77ff1127a4 (read-cache: load cache entries on worker threads,
2018-10-10).

Digging on the mailing list, that parameter was part of an earlier
iteration of the series[1], but became unnecessary when the code
switched to using the IEOT extension.

[1] https://public-inbox.org/git/20180906210227.54368-5-benpeart@microsoft.com/

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

diff --git a/read-cache.c b/read-cache.c
index 61b043bac3..3369f42dd9 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2037,7 +2037,7 @@ static void *load_cache_entries_thread(void *_data)
 }
 
 static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,
-			unsigned long src_offset, int nr_threads, struct index_entry_offset_table *ieot)
+						 int nr_threads, struct index_entry_offset_table *ieot)
 {
 	int i, offset, ieot_blocks, ieot_start, err;
 	struct load_cache_entries_thread_data *data;
@@ -2198,7 +2198,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 		ieot = read_ieot_extension(mmap, mmap_size, extension_offset);
 
 	if (ieot) {
-		src_offset += load_cache_entries_threaded(istate, mmap, mmap_size, src_offset, nr_threads, ieot);
+		src_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);
 		free(ieot);
 	} else {
 		src_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);
-- 
2.21.0.1382.g4c6032d436

[PATCH 06/14] wt-status: drop unused status parameter

[Jeff King]

The v2_fix_up_changed() function doesn't actually need to see the
wt_status struct. It's possible that could change in the future, but
this is a static-local function with one caller. It would be easy to
read-add it back then. Let's drop the unused parameter.

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

diff --git a/wt-status.c b/wt-status.c
index f4fa982638..4c365ca6fd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2045,9 +2045,7 @@ static void wt_porcelain_v2_submodule_state(
 /*
  * Fix-up changed entries before we print them.
  */
-static void wt_porcelain_v2_fix_up_changed(
-	struct string_list_item *it,
-	struct wt_status *s)
+static void wt_porcelain_v2_fix_up_changed(struct string_list_item *it)
 {
 	struct wt_status_change_data *d = it->util;
 
@@ -2107,7 +2105,7 @@ static void wt_porcelain_v2_print_changed_entry(
 	char submodule_token[5];
 	char sep_char, eol_char;
 
-	wt_porcelain_v2_fix_up_changed(it, s);
+	wt_porcelain_v2_fix_up_changed(it);
 	wt_porcelain_v2_submodule_state(d, submodule_token);
 
 	key[0] = d->index_status ? d->index_status : '.';
-- 
2.21.0.1382.g4c6032d436

[PATCH 07/14] mktree: drop unused length parameter

[Jeff King]

The mktree_line() function does not actually look at the "len" parameter
it is passed, and assumes the buffer it receives is NUL-terminated.
Since the caller always passes a strbuf, this will be true. Let's drop
the useless parameter.

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

diff --git a/builtin/mktree.c b/builtin/mktree.c
index 94e82b8504..891991b00d 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -67,7 +67,7 @@ static const char *mktree_usage[] = {
 	NULL
 };
 
-static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_missing)
+static void mktree_line(char *buf, int nul_term_line, int allow_missing)
 {
 	char *ptr, *ntr;
 	const char *p;
@@ -172,7 +172,7 @@ int cmd_mktree(int ac, const char **av, const char *prefix)
 					break;
 				die("input format error: (blank line only valid in batch mode)");
 			}
-			mktree_line(sb.buf, sb.len, nul_term_line, allow_missing);
+			mktree_line(sb.buf, nul_term_line, allow_missing);
 		}
 		if (is_batch_mode && got_eof && used < 1) {
 			/*
-- 
2.21.0.1382.g4c6032d436

[PATCH 08/14] name-rev: drop unused parameters from is_better_name()

[Jeff King]

When this function was extracted in 0041bf6544 (name-rev: refactor logic
to see if a new candidate is a better name, 2017-03-29), it ended up
getting more arguments than it needs.

It's possible we may later use these values to evaluate the name, but
since it's a static function with a single caller, it will be easy to
add them back then.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/name-rev.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 05ccf53e00..16df43473a 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -40,9 +40,7 @@ static void set_commit_rev_name(struct commit *commit, struct rev_name *name)
 }
 
 static int is_better_name(struct rev_name *name,
-			  const char *tip_name,
 			  timestamp_t taggerdate,
-			  int generation,
 			  int distance,
 			  int from_tag)
 {
@@ -103,8 +101,7 @@ static void name_rev(struct commit *commit,
 		name = xmalloc(sizeof(rev_name));
 		set_commit_rev_name(commit, name);
 		goto copy_data;
-	} else if (is_better_name(name, tip_name, taggerdate,
-				  generation, distance, from_tag)) {
+	} else if (is_better_name(name, taggerdate, distance, from_tag)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
-- 
2.21.0.1382.g4c6032d436

[PATCH 09/14] pack-objects: drop unused rev_info parameters

[Jeff King]

When collecting the list of objects to pack in get_object_list(), we
pass our rev_info struct around to some functions that don't need it.
This is due to 03a9683d22 (Simplify is_kept_pack(), 2009-02-28), where
the kept-pack handling was moved out of the revision machinery.

Let's drop these unused parameters.

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

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d7743f110b..fea757fea2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2899,7 +2899,7 @@ static int ofscmp(const void *a_, const void *b_)
 		return oidcmp(&a->object->oid, &b->object->oid);
 }
 
-static void add_objects_in_unpacked_packs(struct rev_info *revs)
+static void add_objects_in_unpacked_packs(void)
 {
 	struct packed_git *p;
 	struct in_pack in_pack;
@@ -3011,7 +3011,7 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,
 	return 1;
 }
 
-static void loosen_unused_packed_objects(struct rev_info *revs)
+static void loosen_unused_packed_objects(void)
 {
 	struct packed_git *p;
 	uint32_t i;
@@ -3158,11 +3158,11 @@ static void get_object_list(int ac, const char **av)
 	}
 
 	if (keep_unreachable)
-		add_objects_in_unpacked_packs(&revs);
+		add_objects_in_unpacked_packs();
 	if (pack_loose_unreachable)
 		add_unreachable_loose_objects();
 	if (unpack_unreachable)
-		loosen_unused_packed_objects(&revs);
+		loosen_unused_packed_objects();
 
 	oid_array_clear(&recent_objects);
 }
-- 
2.21.0.1382.g4c6032d436

[PATCH 10/14] receive-pack: drop unused "commands" from prepare_shallow_update()

[Jeff King]

We pass in the list of proposed ref updates to prepare_shallow_update(),
but that function doesn't actually need it (and never has since its
inception in 0a1bc12b6e4). Only its caller, update_shallow_info(), needs
to look at the command list.

Let's drop the unused parameter to reduce confusion.

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 29f165d8bd..77b7122456 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1809,8 +1809,7 @@ static const char *unpack_with_sideband(struct shallow_info *si)
 	return ret;
 }
 
-static void prepare_shallow_update(struct command *commands,
-				   struct shallow_info *si)
+static void prepare_shallow_update(struct shallow_info *si)
 {
 	int i, j, k, bitmap_size = DIV_ROUND_UP(si->ref->nr, 32);
 
@@ -1876,7 +1875,7 @@ static void update_shallow_info(struct command *commands,
 	si->ref = ref;
 
 	if (shallow_update) {
-		prepare_shallow_update(commands, si);
+		prepare_shallow_update(si);
 		return;
 	}
 
-- 
2.21.0.1382.g4c6032d436

[PATCH 11/14] remove_all_fetch_refspecs(): drop unused "remote" parameter

[Jeff King]

This function already takes a "key" parameter which uniquely identifies
the config key that we need to remove. There's no need for it to look at
the "remote" parameter at all. Let's drop it in the name of simplicity.

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

diff --git a/builtin/remote.c b/builtin/remote.c
index f7edf7f2cb..5591cef775 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1407,7 +1407,7 @@ static int update(int argc, const char **argv)
 	return retval;
 }
 
-static int remove_all_fetch_refspecs(const char *remote, const char *key)
+static int remove_all_fetch_refspecs(const char *key)
 {
 	return git_config_set_multivar_gently(key, NULL, NULL, 1);
 }
@@ -1437,7 +1437,7 @@ static int set_remote_branches(const char *remotename, const char **branches,
 	if (!remote_is_configured(remote, 1))
 		die(_("No such remote '%s'"), remotename);
 
-	if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
+	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
 		strbuf_release(&key);
 		return 1;
 	}
-- 
2.21.0.1382.g4c6032d436

[PATCH 12/14] rev-list: drop unused void pointer from finish_commit()

[Jeff King]

Our finish_commit() function used to be passed directly to the revision
machinery as a callback. But after 989937221a (rev-list: fix
--verify-objects --quiet becoming --objects, 2012-02-28), it is used
only as a helper in show_commit().

It doesn't use its void "data" parameter, and we no longer have to
conform to the callback interface. Let's drop it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rev-list.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 9f31837d30..660172b014 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -77,7 +77,7 @@ static enum missing_action arg_missing_action;
 
 #define DEFAULT_OIDSET_SIZE     (16*1024)
 
-static void finish_commit(struct commit *commit, void *data);
+static void finish_commit(struct commit *commit);
 static void show_commit(struct commit *commit, void *data)
 {
 	struct rev_list_info *info = data;
@@ -86,7 +86,7 @@ static void show_commit(struct commit *commit, void *data)
 	display_progress(progress, ++progress_counter);
 
 	if (info->flags & REV_LIST_QUIET) {
-		finish_commit(commit, data);
+		finish_commit(commit);
 		return;
 	}
 
@@ -99,7 +99,7 @@ static void show_commit(struct commit *commit, void *data)
 			revs->count_left++;
 		else
 			revs->count_right++;
-		finish_commit(commit, data);
+		finish_commit(commit);
 		return;
 	}
 
@@ -188,10 +188,10 @@ static void show_commit(struct commit *commit, void *data)
 			putchar('\n');
 	}
 	maybe_flush_or_die(stdout, "stdout");
-	finish_commit(commit, data);
+	finish_commit(commit);
 }
 
-static void finish_commit(struct commit *commit, void *data)
+static void finish_commit(struct commit *commit)
 {
 	if (commit->parents) {
 		free_commit_list(commit->parents);
-- 
2.21.0.1382.g4c6032d436

[PATCH 13/14] show-branch: drop unused parameter from show_independent()

[Jeff King]

This ref_name parameter was never used since the inception of
show_independent() in 1f8af483df (show-branch: --list and --independent,
2005-09-09). Let's drop it.

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

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 082daeac32..35d7f51c23 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -514,7 +514,6 @@ static int show_merge_base(struct commit_list *seen, int num_rev)
 
 static int show_independent(struct commit **rev,
 			    int num_rev,
-			    char **ref_name,
 			    unsigned int *rev_mask)
 {
 	int i;
@@ -862,7 +861,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		return show_merge_base(seen, num_rev);
 
 	if (independent)
-		return show_independent(rev, num_rev, ref_name, rev_mask);
+		return show_independent(rev, num_rev, rev_mask);
 
 	/* Show list; --more=-1 means list-only */
 	if (1 < num_rev || extra < 0) {
-- 
2.21.0.1382.g4c6032d436

[PATCH 14/14] verify-commit: simplify parameters to run_gpg_verify()

[Jeff King]

The buf/len parameters of run_gpg_verify() have never been used since
the function was added in d07b00b7f3 (verify-commit: scriptable commit
signature verification, 2014-06-23). Instead, check_commit_signature()
accesses the commit struct directly.

Worse, we read the whole object just to check its type and do not attach
it to the "struct commit". Meaning we end up loading the object from
disk twice for no good reason.

And to further confuse matters, our type check is comes from what we
read from disk, but we later assume that lookup_commit() will return
non-NULL. This might not be true if some other object previously
referenced the same oid as a non-commit (though this may be impossible
to trigger in practice since we don't generally parse any other objects
in this command).

Instead, let's do our type check by loading the object via
parse_object(). That will attach the buffer to the struct so it can be
used later by check_commit_signature(). And it ensures that
lookup_commit() will return something sane.

And then we can just drop the unused "buf" and "len" parameters
entirely.

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

diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 7772c07ed7..4b9e823f8f 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -21,15 +21,14 @@ static const char * const verify_commit_usage[] = {
 		NULL
 };
 
-static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned long size, unsigned flags)
+static int run_gpg_verify(struct commit *commit, unsigned flags)
 {
 	struct signature_check signature_check;
 	int ret;
 
 	memset(&signature_check, 0, sizeof(signature_check));
 
-	ret = check_commit_signature(lookup_commit(the_repository, oid),
-				     &signature_check);
+	ret = check_commit_signature(commit, &signature_check);
 	print_signature_buffer(&signature_check, flags);
 
 	signature_check_clear(&signature_check);
@@ -38,26 +37,20 @@ static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned
 
 static int verify_commit(const char *name, unsigned flags)
 {
-	enum object_type type;
 	struct object_id oid;
-	char *buf;
-	unsigned long size;
-	int ret;
+	struct object *obj;
 
 	if (get_oid(name, &oid))
 		return error("commit '%s' not found.", name);
 
-	buf = read_object_file(&oid, &type, &size);
-	if (!buf)
+	obj = parse_object(the_repository, &oid);
+	if (!obj)
 		return error("%s: unable to read file.", name);
-	if (type != OBJ_COMMIT)
+	if (obj->type != OBJ_COMMIT)
 		return error("%s: cannot verify a non-commit object of type %s.",
-				name, type_name(type));
-
-	ret = run_gpg_verify(&oid, buf, size, flags);
+				name, type_name(obj->type));
 
-	free(buf);
-	return ret;
+	return run_gpg_verify((struct commit *)obj, flags);
 }
 
 static int git_verify_commit_config(const char *var, const char *value, void *cb)
-- 
2.21.0.1382.g4c6032d436

Re: [PATCH 01/14] cmd_{read,write}_tree: rename "unused" variable that is used

[Derrick Stolee]

On 5/9/2019 5:27 PM, Jeff King wrote:
> I kind of hate "cmd_prefix"; I was tempted to just call it "prefix" so
> that all of the cmd_* functions were consistent, but I worried that it
> really would get confused with the local variables (even if those
> variables are renamed, as I do here).

cmd_prefix is better than unused_prefix.

The only thing I can think of is "out_prefix" since we are using it
as the equivalent of a C# out-parameter.

-Stolee

Re: [PATCH 03/14] builtin: consistently pass cmd_* prefix to parse_options

[Derrick Stolee]

On 5/9/2019 5:28 PM, Jeff King wrote:
> If a builtin uses RUN_SETUP to request that git.c enter the repository
> directory, we'll get passed in a "prefix" variable with the path to the
> original directory.  It's important to pass this to parse_options(),
> since we may use it to fix up relative OPT_FILENAME() options. Some
> builtins don't bother; let's make sure we do so consistently.
> 
> There may not be any particular bugs fixed here; OPT_FILENAME is
> actually pretty rare, and none of these commands use it directly.
> However, this does future-proof us against somebody adding an option
> that uses it and creating a subtle bug that only shows up when you're in
> a subdirectory of the repository.
> 
> In some cases, like hash-object and upload-pack, we don't specify
> RUN_SETUP, so we know the prefix will always be empty. It's still worth
> passing the variable along to keep the idiom consistent across all
> builtins (and of course it protects us if they ever _did_ switch to
> using RUN_SETUP).

Good explanation that we _should_ be using these parameters. Forgetting
to add these prefix uses would probably not be obvious if a patch added
RUN_SETUP.

Thanks,
-Stolee

Re: [PATCH 14/14] verify-commit: simplify parameters to run_gpg_verify()

[Derrick Stolee]

On 5/9/2019 5:32 PM, Jeff King wrote:
> Instead, let's do our type check by loading the object via
> parse_object(). That will attach the buffer to the struct so it can be
> used later by check_commit_signature(). And it ensures that
> lookup_commit() will return something sane.

This is a good idea.
  
> -static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned long size, unsigned flags)
> +static int run_gpg_verify(struct commit *commit, unsigned flags)
>  {
>  	struct signature_check signature_check;
>  	int ret;
>  
>  	memset(&signature_check, 0, sizeof(signature_check));
>  
> -	ret = check_commit_signature(lookup_commit(the_repository, oid),
> -				     &signature_check);
> +	ret = check_commit_signature(commit, &signature_check);
>  	print_signature_buffer(&signature_check, flags);

Bonus drop of the_repository.

>  
>  	signature_check_clear(&signature_check);
> @@ -38,26 +37,20 @@ static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned
>  
>  static int verify_commit(const char *name, unsigned flags)
>  {
> -	enum object_type type;
>  	struct object_id oid;
> -	char *buf;
> -	unsigned long size;
> -	int ret;
> +	struct object *obj;
>  
>  	if (get_oid(name, &oid))
>  		return error("commit '%s' not found.", name);
>  
> -	buf = read_object_file(&oid, &type, &size);
> -	if (!buf)
> +	obj = parse_object(the_repository, &oid);

...and it is back, but "higher" up. That's fine.

> +	if (!obj)
>  		return error("%s: unable to read file.", name);
> -	if (type != OBJ_COMMIT)
> +	if (obj->type != OBJ_COMMIT)
>  		return error("%s: cannot verify a non-commit object of type %s.",
> -				name, type_name(type));
> -
> -	ret = run_gpg_verify(&oid, buf, size, flags);
> +				name, type_name(obj->type));
>  
> -	free(buf);
> -	return ret;
> +	return run_gpg_verify((struct commit *)obj, flags);
>  }

You pointed out that you thought this patch was subtle. I agree that
there is a slightly changed functionality, but it seems to be
improving a possibly wrong behavior.

Thanks,
-Stolee

Re: [PATCH 0/14] "final" batch of unused parameter cleanups

[Derrick Stolee]

On 5/9/2019 5:25 PM, Jeff King wrote:
> This is a continuation of my efforts to get us compiling with
> -Wunused-parameter.  This round finishes most of the cleanups and fixes
> I intend to do (though I have a handful of cleanup cases that I'm still
> poking at to make sure they're not in fact bugs). After that, I have
> some patches to annotate unused parameters we can't get rid of (e.g.,
> for cases where we're conforming to a callback interface), and then we
> can finally flip the warning on for developer-mode.

This series looks correct to me. I look forward to having the warning
on in developer mode!

Thanks,
-Stolee

Re: [PATCH 01/14] cmd_{read,write}_tree: rename "unused" variable that is used

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I kind of hate "cmd_prefix"; I was tempted to just call it "prefix" so
> that all of the cmd_* functions were consistent, but I worried that it
> really would get confused with the local variables (even if those
> variables are renamed, as I do here).

I tend to agree with your temptation to make things consisten,
especially because you already do the renaming of existing "prefix"
to "tree_prefix".

Perhaps in a few release cycles, these can be renamed again to be
consistent with everybody else, and I won't die if this were called
cmd_prefix until then ;-)