{"thread":{"id":"51062","subject":"[PATCH 0/14] \"final\" batch of unused parameter cleanups","startedAt":"2019-05-09T21:26:02Z","lastAt":"2019-05-13T05:14:19Z","messageCount":20,"participants":["Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":14},"messages":[{"id":"375256","messageId":"20190509212558.GA15438@sigill.intra.peff.net","threadId":"51062","inReplyTo":null,"subject":"[PATCH 0/14] \"final\" batch of unused parameter cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:25:59Z","receivedAt":"2019-05-09T21:26:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a continuation of my efforts to get us compiling with\n-Wunused-parameter.  This round finishes most of the cleanups and fixes\nI intend to do (though I have a handful of cleanup cases that I'm still\npoking at to make sure they're not in fact bugs). After that, I have\nsome patches to annotate unused parameters we can't get rid of (e.g.,\nfor cases where we're conforming to a callback interface), and then we\ncan finally flip the warning on for developer-mode.\n\nThis series has two small conflicts with what's in \"pu\":\n\n  - there's a minor textual conflict in upload-pack\n\n  - it changes cmd_rebase__interactive(), which moved into\n    builtin/rebase.c in pw/rebase-i-internal. Because the old file went\n    away, this results in a delete/modified conflict, and you have to\n    manually fix up the result, like so:\n\ndiff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c\ndeleted file mode 100644\nindex 72fd4b53a8..0000000000\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8c2cbc72f6..cc1e1a997d 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -508,7 +508,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)\n \tif (argc == 1)\n \t\tusage_with_options(builtin_rebase_interactive_usage, options);\n \n-\targc = parse_options(argc, argv, NULL, options,\n+\targc = parse_options(argc, argv, prefix, options,\n \t\t\tbuiltin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);\n \n \tif (!is_null_oid(&squash_onto))\n\nMost of the patches are pretty straightforward and won't have any\nbehavior change (the thing to review in them is confirming that not\nusing the parameter is the right thing to do; I've tried to make a case\nfor each in the commit message). The trickiest one is patch 14.\n\n  [01/14]: cmd_{read,write}_tree: rename \"unused\" variable that is used\n  [02/14]: submodule: drop unused prefix parameter from some functions\n  [03/14]: builtin: consistently pass cmd_* prefix to parse_options\n  [04/14]: clone: drop dest parameter from copy_alternates()\n  [05/14]: read-cache: drop unused parameter from threaded load\n  [06/14]: wt-status: drop unused status parameter\n  [07/14]: mktree: drop unused length parameter\n  [08/14]: name-rev: drop unused parameters from is_better_name()\n  [09/14]: pack-objects: drop unused rev_info parameters\n  [10/14]: receive-pack: drop unused \"commands\" from prepare_shallow_update()\n  [11/14]: remove_all_fetch_refspecs(): drop unused \"remote\" parameter\n  [12/14]: rev-list: drop unused void pointer from finish_commit()\n  [13/14]: show-branch: drop unused parameter from show_independent()\n  [14/14]: verify-commit: simplify parameters to run_gpg_verify()\n\n builtin/clone.c               |  5 ++---\n builtin/column.c              |  2 +-\n builtin/hash-object.c         |  2 +-\n builtin/mktree.c              |  4 ++--\n builtin/name-rev.c            |  5 +----\n builtin/pack-objects.c        |  8 ++++----\n builtin/range-diff.c          |  2 +-\n builtin/read-tree.c           |  4 ++--\n builtin/rebase--interactive.c |  2 +-\n builtin/receive-pack.c        |  5 ++---\n builtin/remote.c              |  4 ++--\n builtin/rev-list.c            | 10 +++++-----\n builtin/rm.c                  |  6 +++---\n builtin/show-branch.c         |  3 +--\n builtin/submodule--helper.c   |  3 +--\n builtin/upload-pack.c         |  2 +-\n builtin/verify-commit.c       | 23 ++++++++---------------\n builtin/write-tree.c          | 12 ++++++------\n read-cache.c                  |  4 ++--\n submodule.c                   | 10 ++++------\n submodule.h                   |  3 +--\n wt-status.c                   |  6 ++----\n 22 files changed, 53 insertions(+), 72 deletions(-)\n\n"},{"id":"375257","messageId":"20190509212724.GA15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 01/14] cmd_{read,write}_tree: rename \"unused\" variable that is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:27:24Z","receivedAt":"2019-05-09T21:27:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"prefix\" variable passed by git.c into the builtin cmd_read_tree()\nand cmd_write_tree() functions is named \"unused_prefix\". But we do in\nfact pass it to parse_options(), which may use the prefix to adjust any\nfilename options. Let's get rid of this confusing name.\n\nHowever, we can't just call it \"prefix\". The reason these variables were\nrenamed in the first place is that they shadowed local variables named\n\"prefix\", because these commands both take a \"--prefix\" option.\n\nSo let's rename the parameters, but try to reduce further confusion:\n\n  1. In both cases we'll call them \"cmd_prefix\" to mark that they're\n     part of the cmd_* interface.\n\n  2. In cmd_write_tree(), we'll rename the local prefix variable to\n     \"tree_prefix\" to make it more clear that we're talking about the\n     prefix to be used for the tree we're writing.\n\n  3. In cmd_read_tree(), the \"prefix\" local has since migrated into\n     \"struct unpack_trees_options\". We'll leave that alone, as the\n     context within the struct makes its meaning clear (we actually\n     _could_ just call the parameter \"prefix\" now, but that invites\n     confusion in the other direction).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI kind of hate \"cmd_prefix\"; I was tempted to just call it \"prefix\" so\nthat all of the cmd_* functions were consistent, but I worried that it\nreally would get confused with the local variables (even if those\nvariables are renamed, as I do here).\n\n builtin/read-tree.c  |  4 ++--\n builtin/write-tree.c | 12 ++++++------\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 5c9c082595..ca5e655d2f 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -111,7 +111,7 @@ static int git_read_tree_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n+int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)\n {\n \tint i, stage = 0;\n \tstruct object_id oid;\n@@ -165,7 +165,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \n \tgit_config(git_read_tree_config, NULL);\n \n-\targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n+\targc = parse_options(argc, argv, cmd_prefix, read_tree_options,\n \t\t\t     read_tree_usage, 0);\n \n \thold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);\ndiff --git a/builtin/write-tree.c b/builtin/write-tree.c\nindex 3d46d22ee5..45d61707e7 100644\n--- a/builtin/write-tree.c\n+++ b/builtin/write-tree.c\n@@ -16,16 +16,16 @@ static const char * const write_tree_usage[] = {\n \tNULL\n };\n \n-int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)\n+int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)\n {\n \tint flags = 0, ret;\n-\tconst char *prefix = NULL;\n+\tconst char *tree_prefix = NULL;\n \tstruct object_id oid;\n \tconst char *me = \"git-write-tree\";\n \tstruct option write_tree_options[] = {\n \t\tOPT_BIT(0, \"missing-ok\", &flags, N_(\"allow missing objects\"),\n \t\t\tWRITE_TREE_MISSING_OK),\n-\t\tOPT_STRING(0, \"prefix\", &prefix, N_(\"<prefix>/\"),\n+\t\tOPT_STRING(0, \"prefix\", &tree_prefix, N_(\"<prefix>/\"),\n \t\t\t   N_(\"write tree object for a subdirectory <prefix>\")),\n \t\t{ OPTION_BIT, 0, \"ignore-cache-tree\", &flags, NULL,\n \t\t  N_(\"only useful for debugging\"),\n@@ -35,10 +35,10 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)\n \t};\n \n \tgit_config(git_default_config, NULL);\n-\targc = parse_options(argc, argv, unused_prefix, write_tree_options,\n+\targc = parse_options(argc, argv, cmd_prefix, write_tree_options,\n \t\t\t     write_tree_usage, 0);\n \n-\tret = write_cache_as_tree(&oid, flags, prefix);\n+\tret = write_cache_as_tree(&oid, flags, tree_prefix);\n \tswitch (ret) {\n \tcase 0:\n \t\tprintf(\"%s\\n\", oid_to_hex(&oid));\n@@ -50,7 +50,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)\n \t\tdie(\"%s: error building trees\", me);\n \t\tbreak;\n \tcase WRITE_TREE_PREFIX_ERROR:\n-\t\tdie(\"%s: prefix %s not found\", me, prefix);\n+\t\tdie(\"%s: prefix %s not found\", me, tree_prefix);\n \t\tbreak;\n \t}\n \treturn ret;\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375258","messageId":"20190509212731.GB15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 02/14] submodule: drop unused prefix parameter from some functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:27:31Z","receivedAt":"2019-05-09T21:27:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We stopped using the \"prefix\" parameter of\nrelocate_single_git_dir_into_superproject() and its callers in\n202275b96b (submodule.c: get_super_prefix_or_empty, 2017-03-14), where\nwe switched to using the environment global directly.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rm.c                |  6 +++---\n builtin/submodule--helper.c |  3 +--\n submodule.c                 | 10 ++++------\n submodule.h                 |  3 +--\n 4 files changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 90cbe896c9..be8edc6d1e 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -61,7 +61,7 @@ static void print_error_files(struct string_list *files_list,\n \t}\n }\n \n-static void submodules_absorb_gitdir_if_needed(const char *prefix)\n+static void submodules_absorb_gitdir_if_needed(void)\n {\n \tint i;\n \tfor (i = 0; i < list.nr; i++) {\n@@ -83,7 +83,7 @@ static void submodules_absorb_gitdir_if_needed(const char *prefix)\n \t\t\tcontinue;\n \n \t\tif (!submodule_uses_gitfile(name))\n-\t\t\tabsorb_git_dir_into_superproject(prefix, name,\n+\t\t\tabsorb_git_dir_into_superproject(name,\n \t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n \t}\n }\n@@ -313,7 +313,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!index_only)\n-\t\tsubmodules_absorb_gitdir_if_needed(prefix);\n+\t\tsubmodules_absorb_gitdir_if_needed();\n \n \t/*\n \t * If not forced, the file, the index and the HEAD (if exists)\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 8c72ea864c..7262f1a000 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2107,8 +2107,7 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)\n \t\treturn 1;\n \n \tfor (i = 0; i < list.nr; i++)\n-\t\tabsorb_git_dir_into_superproject(prefix,\n-\t\t\t\tlist.entries[i]->name, flags);\n+\t\tabsorb_git_dir_into_superproject(list.entries[i]->name, flags);\n \n \treturn 0;\n }\ndiff --git a/submodule.c b/submodule.c\nindex 2cfaba0599..0f199c5137 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1910,7 +1910,7 @@ int submodule_move_head(const char *path,\n \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n \t\tif (old_head) {\n \t\t\tif (!submodule_uses_gitfile(path))\n-\t\t\t\tabsorb_git_dir_into_superproject(\"\", path,\n+\t\t\t\tabsorb_git_dir_into_superproject(path,\n \t\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n \t\t} else {\n \t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n@@ -1997,8 +1997,7 @@ int submodule_move_head(const char *path,\n  * Embeds a single submodules git directory into the superprojects git dir,\n  * non recursively.\n  */\n-static void relocate_single_git_dir_into_superproject(const char *prefix,\n-\t\t\t\t\t\t      const char *path)\n+static void relocate_single_git_dir_into_superproject(const char *path)\n {\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n \tconst char *new_git_dir;\n@@ -2040,8 +2039,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n  * having its git directory within the working tree to the git dir nested\n  * in its superprojects git dir under modules/.\n  */\n-void absorb_git_dir_into_superproject(const char *prefix,\n-\t\t\t\t      const char *path,\n+void absorb_git_dir_into_superproject(const char *path,\n \t\t\t\t      unsigned flags)\n {\n \tint err_code;\n@@ -2082,7 +2080,7 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\tchar *real_common_git_dir = real_pathdup(get_git_common_dir(), 1);\n \n \t\tif (!starts_with(real_sub_git_dir, real_common_git_dir))\n-\t\t\trelocate_single_git_dir_into_superproject(prefix, path);\n+\t\t\trelocate_single_git_dir_into_superproject(path);\n \n \t\tfree(real_sub_git_dir);\n \t\tfree(real_common_git_dir);\ndiff --git a/submodule.h b/submodule.h\nindex 9e18e9b807..8072e6d6dd 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -141,8 +141,7 @@ void submodule_unset_core_worktree(const struct submodule *sub);\n void prepare_submodule_repo_env(struct argv_array *out);\n \n #define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)\n-void absorb_git_dir_into_superproject(const char *prefix,\n-\t\t\t\t      const char *path,\n+void absorb_git_dir_into_superproject(const char *path,\n \t\t\t\t      unsigned flags);\n \n /*\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375259","messageId":"20190509212850.GC15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 03/14] builtin: consistently pass cmd_* prefix to parse_options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:28:51Z","receivedAt":"2019-05-09T21:28:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a builtin uses RUN_SETUP to request that git.c enter the repository\ndirectory, we'll get passed in a \"prefix\" variable with the path to the\noriginal directory.  It's important to pass this to parse_options(),\nsince we may use it to fix up relative OPT_FILENAME() options. Some\nbuiltins don't bother; let's make sure we do so consistently.\n\nThere may not be any particular bugs fixed here; OPT_FILENAME is\nactually pretty rare, and none of these commands use it directly.\nHowever, this does future-proof us against somebody adding an option\nthat uses it and creating a subtle bug that only shows up when you're in\na subdirectory of the repository.\n\nIn some cases, like hash-object and upload-pack, we don't specify\nRUN_SETUP, so we know the prefix will always be empty. It's still worth\npassing the variable along to keep the idiom consistent across all\nbuiltins (and of course it protects us if they ever _did_ switch to\nusing RUN_SETUP).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/column.c              | 2 +-\n builtin/hash-object.c         | 2 +-\n builtin/range-diff.c          | 2 +-\n builtin/rebase--interactive.c | 2 +-\n builtin/upload-pack.c         | 2 +-\n 5 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/column.c b/builtin/column.c\nindex 5228ccf37a..e815e148aa 100644\n--- a/builtin/column.c\n+++ b/builtin/column.c\n@@ -43,7 +43,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)\n \n \tmemset(&copts, 0, sizeof(copts));\n \tcopts.padding = 1;\n-\targc = parse_options(argc, argv, \"\", options, builtin_column_usage, 0);\n+\targc = parse_options(argc, argv, prefix, options, builtin_column_usage, 0);\n \tif (argc)\n \t\tusage_with_options(builtin_column_usage, options);\n \tif (real_command || command) {\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex e055c11103..640ef4ded5 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -108,7 +108,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \tint i;\n \tconst char *errstr = NULL;\n \n-\targc = parse_options(argc, argv, NULL, hash_object_options,\n+\targc = parse_options(argc, argv, prefix, hash_object_options,\n \t\t\t     hash_object_usage, 0);\n \n \tif (flags & HASH_WRITE_OBJECT)\ndiff --git a/builtin/range-diff.c b/builtin/range-diff.c\nindex 784bd19321..9202e75544 100644\n--- a/builtin/range-diff.c\n+++ b/builtin/range-diff.c\n@@ -32,7 +32,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)\n \trepo_diff_setup(the_repository, &diffopt);\n \n \toptions = parse_options_concat(range_diff_options, diffopt.parseopts);\n-\targc = parse_options(argc, argv, NULL, options,\n+\targc = parse_options(argc, argv, prefix, options,\n \t\t\t     builtin_range_diff_usage, 0);\n \n \tdiff_setup_done(&diffopt);\ndiff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c\nindex 4535523bf5..72fd4b53a8 100644\n--- a/builtin/rebase--interactive.c\n+++ b/builtin/rebase--interactive.c\n@@ -302,7 +302,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)\n \tif (argc == 1)\n \t\tusage_with_options(builtin_rebase_interactive_usage, options);\n \n-\targc = parse_options(argc, argv, NULL, options,\n+\targc = parse_options(argc, argv, prefix, options,\n \t\t\tbuiltin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);\n \n \topts.gpg_sign = xstrdup_or_null(opts.gpg_sign);\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 42dc4da5a1..6da8fa2607 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -33,7 +33,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \tpacket_trace_identity(\"upload-pack\");\n \tread_replace_refs = 0;\n \n-\targc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);\n+\targc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);\n \n \tif (argc != 1)\n \t\tusage_with_options(upload_pack_usage, options);\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375260","messageId":"20190509212922.GD15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 04/14] clone: drop dest parameter from copy_alternates()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:29:22Z","receivedAt":"2019-05-09T21:29:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Ever since the inception of this function in e6baf4a1ae (clone: clone\nfrom a repository with relative alternates, 2011-08-22), the \"dest\"\nparameter has been unused. Instead, we use add_to_alternates_file(),\nwhich relies on git_pathdup() to find the right file. That in turn works\nbecause we will have initialized and entered the destination repo by\nthis point.\n\nIt's a bit subtle, but this is how it has always worked. And if our\nassumptions change, the test in t5601 from e6baf4a1ae should let us\nknow.\n\nIn the meantime, let's drop this unused and confusing parameter from\ncopy_alternates().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 31a47d190a..9ddf4f99e5 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -357,8 +357,7 @@ static void setup_reference(void)\n \t\t\t     add_one_reference, &required);\n }\n \n-static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n-\t\t\t    const char *src_repo)\n+static void copy_alternates(struct strbuf *src, const char *src_repo)\n {\n \t/*\n \t * Read from the source objects/info/alternates file\n@@ -439,7 +438,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,\n \n \t\t/* Files that cannot be copied bit-for-bit... */\n \t\tif (!strcmp(src->buf + src_baselen, \"/info/alternates\")) {\n-\t\t\tcopy_alternates(src, dest, src_repo);\n+\t\t\tcopy_alternates(src, src_repo);\n \t\t\tcontinue;\n \t\t}\n \n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375261","messageId":"20190509212944.GE15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 05/14] read-cache: drop unused parameter from threaded load","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:29:44Z","receivedAt":"2019-05-09T21:29:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The load_cache_entries_threaded() function takes a src_offset parameter\nthat it doesn't use. This has been there since its inception in\n77ff1127a4 (read-cache: load cache entries on worker threads,\n2018-10-10).\n\nDigging on the mailing list, that parameter was part of an earlier\niteration of the series[1], but became unnecessary when the code\nswitched to using the IEOT extension.\n\n[1] https://public-inbox.org/git/20180906210227.54368-5-benpeart@microsoft.com/\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n read-cache.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 61b043bac3..3369f42dd9 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2037,7 +2037,7 @@ static void *load_cache_entries_thread(void *_data)\n }\n \n static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n-\t\t\tunsigned long src_offset, int nr_threads, struct index_entry_offset_table *ieot)\n+\t\t\t\t\t\t int nr_threads, struct index_entry_offset_table *ieot)\n {\n \tint i, offset, ieot_blocks, ieot_start, err;\n \tstruct load_cache_entries_thread_data *data;\n@@ -2198,7 +2198,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \t\tieot = read_ieot_extension(mmap, mmap_size, extension_offset);\n \n \tif (ieot) {\n-\t\tsrc_offset += load_cache_entries_threaded(istate, mmap, mmap_size, src_offset, nr_threads, ieot);\n+\t\tsrc_offset += load_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);\n \t\tfree(ieot);\n \t} else {\n \t\tsrc_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375262","messageId":"20190509213019.GF15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 06/14] wt-status: drop unused status parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:30:19Z","receivedAt":"2019-05-09T21:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The v2_fix_up_changed() function doesn't actually need to see the\nwt_status struct. It's possible that could change in the future, but\nthis is a static-local function with one caller. It would be easy to\nread-add it back then. Let's drop the unused parameter.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wt-status.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex f4fa982638..4c365ca6fd 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2045,9 +2045,7 @@ static void wt_porcelain_v2_submodule_state(\n /*\n  * Fix-up changed entries before we print them.\n  */\n-static void wt_porcelain_v2_fix_up_changed(\n-\tstruct string_list_item *it,\n-\tstruct wt_status *s)\n+static void wt_porcelain_v2_fix_up_changed(struct string_list_item *it)\n {\n \tstruct wt_status_change_data *d = it->util;\n \n@@ -2107,7 +2105,7 @@ static void wt_porcelain_v2_print_changed_entry(\n \tchar submodule_token[5];\n \tchar sep_char, eol_char;\n \n-\twt_porcelain_v2_fix_up_changed(it, s);\n+\twt_porcelain_v2_fix_up_changed(it);\n \twt_porcelain_v2_submodule_state(d, submodule_token);\n \n \tkey[0] = d->index_status ? d->index_status : '.';\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375263","messageId":"20190509213037.GG15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 07/14] mktree: drop unused length parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:30:37Z","receivedAt":"2019-05-09T21:30:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The mktree_line() function does not actually look at the \"len\" parameter\nit is passed, and assumes the buffer it receives is NUL-terminated.\nSince the caller always passes a strbuf, this will be true. Let's drop\nthe useless parameter.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/mktree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mktree.c b/builtin/mktree.c\nindex 94e82b8504..891991b00d 100644\n--- a/builtin/mktree.c\n+++ b/builtin/mktree.c\n@@ -67,7 +67,7 @@ static const char *mktree_usage[] = {\n \tNULL\n };\n \n-static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_missing)\n+static void mktree_line(char *buf, int nul_term_line, int allow_missing)\n {\n \tchar *ptr, *ntr;\n \tconst char *p;\n@@ -172,7 +172,7 @@ int cmd_mktree(int ac, const char **av, const char *prefix)\n \t\t\t\t\tbreak;\n \t\t\t\tdie(\"input format error: (blank line only valid in batch mode)\");\n \t\t\t}\n-\t\t\tmktree_line(sb.buf, sb.len, nul_term_line, allow_missing);\n+\t\t\tmktree_line(sb.buf, nul_term_line, allow_missing);\n \t\t}\n \t\tif (is_batch_mode && got_eof && used < 1) {\n \t\t\t/*\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375264","messageId":"20190509213056.GH15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 08/14] name-rev: drop unused parameters from is_better_name()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:30:56Z","receivedAt":"2019-05-09T21:30:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When this function was extracted in 0041bf6544 (name-rev: refactor logic\nto see if a new candidate is a better name, 2017-03-29), it ended up\ngetting more arguments than it needs.\n\nIt's possible we may later use these values to evaluate the name, but\nsince it's a static function with a single caller, it will be easy to\nadd them back then.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/name-rev.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 05ccf53e00..16df43473a 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -40,9 +40,7 @@ static void set_commit_rev_name(struct commit *commit, struct rev_name *name)\n }\n \n static int is_better_name(struct rev_name *name,\n-\t\t\t  const char *tip_name,\n \t\t\t  timestamp_t taggerdate,\n-\t\t\t  int generation,\n \t\t\t  int distance,\n \t\t\t  int from_tag)\n {\n@@ -103,8 +101,7 @@ static void name_rev(struct commit *commit,\n \t\tname = xmalloc(sizeof(rev_name));\n \t\tset_commit_rev_name(commit, name);\n \t\tgoto copy_data;\n-\t} else if (is_better_name(name, tip_name, taggerdate,\n-\t\t\t\t  generation, distance, from_tag)) {\n+\t} else if (is_better_name(name, taggerdate, distance, from_tag)) {\n copy_data:\n \t\tname->tip_name = tip_name;\n \t\tname->taggerdate = taggerdate;\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375265","messageId":"20190509213116.GI15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 09/14] pack-objects: drop unused rev_info parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:31:16Z","receivedAt":"2019-05-09T21:31:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When collecting the list of objects to pack in get_object_list(), we\npass our rev_info struct around to some functions that don't need it.\nThis is due to 03a9683d22 (Simplify is_kept_pack(), 2009-02-28), where\nthe kept-pack handling was moved out of the revision machinery.\n\nLet's drop these unused parameters.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d7743f110b..fea757fea2 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2899,7 +2899,7 @@ static int ofscmp(const void *a_, const void *b_)\n \t\treturn oidcmp(&a->object->oid, &b->object->oid);\n }\n \n-static void add_objects_in_unpacked_packs(struct rev_info *revs)\n+static void add_objects_in_unpacked_packs(void)\n {\n \tstruct packed_git *p;\n \tstruct in_pack in_pack;\n@@ -3011,7 +3011,7 @@ static int loosened_object_can_be_discarded(const struct object_id *oid,\n \treturn 1;\n }\n \n-static void loosen_unused_packed_objects(struct rev_info *revs)\n+static void loosen_unused_packed_objects(void)\n {\n \tstruct packed_git *p;\n \tuint32_t i;\n@@ -3158,11 +3158,11 @@ static void get_object_list(int ac, const char **av)\n \t}\n \n \tif (keep_unreachable)\n-\t\tadd_objects_in_unpacked_packs(&revs);\n+\t\tadd_objects_in_unpacked_packs();\n \tif (pack_loose_unreachable)\n \t\tadd_unreachable_loose_objects();\n \tif (unpack_unreachable)\n-\t\tloosen_unused_packed_objects(&revs);\n+\t\tloosen_unused_packed_objects();\n \n \toid_array_clear(&recent_objects);\n }\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375266","messageId":"20190509213138.GJ15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 10/14] receive-pack: drop unused \"commands\" from prepare_shallow_update()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:31:39Z","receivedAt":"2019-05-09T21:31:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pass in the list of proposed ref updates to prepare_shallow_update(),\nbut that function doesn't actually need it (and never has since its\ninception in 0a1bc12b6e4). Only its caller, update_shallow_info(), needs\nto look at the command list.\n\nLet's drop the unused parameter to reduce confusion.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 29f165d8bd..77b7122456 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1809,8 +1809,7 @@ static const char *unpack_with_sideband(struct shallow_info *si)\n \treturn ret;\n }\n \n-static void prepare_shallow_update(struct command *commands,\n-\t\t\t\t   struct shallow_info *si)\n+static void prepare_shallow_update(struct shallow_info *si)\n {\n \tint i, j, k, bitmap_size = DIV_ROUND_UP(si->ref->nr, 32);\n \n@@ -1876,7 +1875,7 @@ static void update_shallow_info(struct command *commands,\n \tsi->ref = ref;\n \n \tif (shallow_update) {\n-\t\tprepare_shallow_update(commands, si);\n+\t\tprepare_shallow_update(si);\n \t\treturn;\n \t}\n \n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375267","messageId":"20190509213146.GK15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 11/14] remove_all_fetch_refspecs(): drop unused \"remote\" parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:31:46Z","receivedAt":"2019-05-09T21:31:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function already takes a \"key\" parameter which uniquely identifies\nthe config key that we need to remove. There's no need for it to look at\nthe \"remote\" parameter at all. Let's drop it in the name of simplicity.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/remote.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f7edf7f2cb..5591cef775 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1407,7 +1407,7 @@ static int update(int argc, const char **argv)\n \treturn retval;\n }\n \n-static int remove_all_fetch_refspecs(const char *remote, const char *key)\n+static int remove_all_fetch_refspecs(const char *key)\n {\n \treturn git_config_set_multivar_gently(key, NULL, NULL, 1);\n }\n@@ -1437,7 +1437,7 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \n-\tif (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {\n+\tif (!add_mode && remove_all_fetch_refspecs(key.buf)) {\n \t\tstrbuf_release(&key);\n \t\treturn 1;\n \t}\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375268","messageId":"20190509213203.GL15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 12/14] rev-list: drop unused void pointer from finish_commit()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:32:03Z","receivedAt":"2019-05-09T21:32:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our finish_commit() function used to be passed directly to the revision\nmachinery as a callback. But after 989937221a (rev-list: fix\n--verify-objects --quiet becoming --objects, 2012-02-28), it is used\nonly as a helper in show_commit().\n\nIt doesn't use its void \"data\" parameter, and we no longer have to\nconform to the callback interface. Let's drop it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-list.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 9f31837d30..660172b014 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -77,7 +77,7 @@ static enum missing_action arg_missing_action;\n \n #define DEFAULT_OIDSET_SIZE     (16*1024)\n \n-static void finish_commit(struct commit *commit, void *data);\n+static void finish_commit(struct commit *commit);\n static void show_commit(struct commit *commit, void *data)\n {\n \tstruct rev_list_info *info = data;\n@@ -86,7 +86,7 @@ static void show_commit(struct commit *commit, void *data)\n \tdisplay_progress(progress, ++progress_counter);\n \n \tif (info->flags & REV_LIST_QUIET) {\n-\t\tfinish_commit(commit, data);\n+\t\tfinish_commit(commit);\n \t\treturn;\n \t}\n \n@@ -99,7 +99,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\t\trevs->count_left++;\n \t\telse\n \t\t\trevs->count_right++;\n-\t\tfinish_commit(commit, data);\n+\t\tfinish_commit(commit);\n \t\treturn;\n \t}\n \n@@ -188,10 +188,10 @@ static void show_commit(struct commit *commit, void *data)\n \t\t\tputchar('\\n');\n \t}\n \tmaybe_flush_or_die(stdout, \"stdout\");\n-\tfinish_commit(commit, data);\n+\tfinish_commit(commit);\n }\n \n-static void finish_commit(struct commit *commit, void *data)\n+static void finish_commit(struct commit *commit)\n {\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375269","messageId":"20190509213213.GM15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 13/14] show-branch: drop unused parameter from show_independent()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:32:14Z","receivedAt":"2019-05-09T21:32:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This ref_name parameter was never used since the inception of\nshow_independent() in 1f8af483df (show-branch: --list and --independent,\n2005-09-09). Let's drop it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/show-branch.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 082daeac32..35d7f51c23 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -514,7 +514,6 @@ static int show_merge_base(struct commit_list *seen, int num_rev)\n \n static int show_independent(struct commit **rev,\n \t\t\t    int num_rev,\n-\t\t\t    char **ref_name,\n \t\t\t    unsigned int *rev_mask)\n {\n \tint i;\n@@ -862,7 +861,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\treturn show_merge_base(seen, num_rev);\n \n \tif (independent)\n-\t\treturn show_independent(rev, num_rev, ref_name, rev_mask);\n+\t\treturn show_independent(rev, num_rev, rev_mask);\n \n \t/* Show list; --more=-1 means list-only */\n \tif (1 < num_rev || extra < 0) {\n-- \n2.21.0.1382.g4c6032d436\n\n"},{"id":"375270","messageId":"20190509213229.GN15837@sigill.intra.peff.net","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"[PATCH 14/14] verify-commit: simplify parameters to run_gpg_verify()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-09T21:32:29Z","receivedAt":"2019-05-09T21:32:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The buf/len parameters of run_gpg_verify() have never been used since\nthe function was added in d07b00b7f3 (verify-commit: scriptable commit\nsignature verification, 2014-06-23). Instead, check_commit_signature()\naccesses the commit struct directly.\n\nWorse, we read the whole object just to check its type and do not attach\nit to the \"struct commit\". Meaning we end up loading the object from\ndisk twice for no good reason.\n\nAnd to further confuse matters, our type check is comes from what we\nread from disk, but we later assume that lookup_commit() will return\nnon-NULL. This might not be true if some other object previously\nreferenced the same oid as a non-commit (though this may be impossible\nto trigger in practice since we don't generally parse any other objects\nin this command).\n\nInstead, let's do our type check by loading the object via\nparse_object(). That will attach the buffer to the struct so it can be\nused later by check_commit_signature(). And it ensures that\nlookup_commit() will return something sane.\n\nAnd then we can just drop the unused \"buf\" and \"len\" parameters\nentirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/verify-commit.c | 23 ++++++++---------------\n 1 file changed, 8 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/verify-commit.c b/builtin/verify-commit.c\nindex 7772c07ed7..4b9e823f8f 100644\n--- a/builtin/verify-commit.c\n+++ b/builtin/verify-commit.c\n@@ -21,15 +21,14 @@ static const char * const verify_commit_usage[] = {\n \t\tNULL\n };\n \n-static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned long size, unsigned flags)\n+static int run_gpg_verify(struct commit *commit, unsigned flags)\n {\n \tstruct signature_check signature_check;\n \tint ret;\n \n \tmemset(&signature_check, 0, sizeof(signature_check));\n \n-\tret = check_commit_signature(lookup_commit(the_repository, oid),\n-\t\t\t\t     &signature_check);\n+\tret = check_commit_signature(commit, &signature_check);\n \tprint_signature_buffer(&signature_check, flags);\n \n \tsignature_check_clear(&signature_check);\n@@ -38,26 +37,20 @@ static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned\n \n static int verify_commit(const char *name, unsigned flags)\n {\n-\tenum object_type type;\n \tstruct object_id oid;\n-\tchar *buf;\n-\tunsigned long size;\n-\tint ret;\n+\tstruct object *obj;\n \n \tif (get_oid(name, &oid))\n \t\treturn error(\"commit '%s' not found.\", name);\n \n-\tbuf = read_object_file(&oid, &type, &size);\n-\tif (!buf)\n+\tobj = parse_object(the_repository, &oid);\n+\tif (!obj)\n \t\treturn error(\"%s: unable to read file.\", name);\n-\tif (type != OBJ_COMMIT)\n+\tif (obj->type != OBJ_COMMIT)\n \t\treturn error(\"%s: cannot verify a non-commit object of type %s.\",\n-\t\t\t\tname, type_name(type));\n-\n-\tret = run_gpg_verify(&oid, buf, size, flags);\n+\t\t\t\tname, type_name(obj->type));\n \n-\tfree(buf);\n-\treturn ret;\n+\treturn run_gpg_verify((struct commit *)obj, flags);\n }\n \n static int git_verify_commit_config(const char *var, const char *value, void *cb)\n-- \n2.21.0.1382.g4c6032d436\n"},{"id":"375287","messageId":"e05b6872-3078-afca-bb12-91d628f9c44b@gmail.com","threadId":"51062","inReplyTo":"20190509212724.GA15837@sigill.intra.peff.net","subject":"Re: [PATCH 01/14] cmd_{read,write}_tree: rename \"unused\" variable that is used","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-10T12:07:58Z","receivedAt":"2019-05-10T12:08:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/9/2019 5:27 PM, Jeff King wrote:\n> I kind of hate \"cmd_prefix\"; I was tempted to just call it \"prefix\" so\n> that all of the cmd_* functions were consistent, but I worried that it\n> really would get confused with the local variables (even if those\n> variables are renamed, as I do here).\n\ncmd_prefix is better than unused_prefix.\n\nThe only thing I can think of is \"out_prefix\" since we are using it\nas the equivalent of a C# out-parameter.\n\n-Stolee\n\n"},{"id":"375288","messageId":"3851512b-9957-8e77-f4f7-3a859f808676@gmail.com","threadId":"51062","inReplyTo":"20190509212850.GC15837@sigill.intra.peff.net","subject":"Re: [PATCH 03/14] builtin: consistently pass cmd_* prefix to parse_options","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-10T12:10:49Z","receivedAt":"2019-05-10T12:10:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/9/2019 5:28 PM, Jeff King wrote:\n> If a builtin uses RUN_SETUP to request that git.c enter the repository\n> directory, we'll get passed in a \"prefix\" variable with the path to the\n> original directory.  It's important to pass this to parse_options(),\n> since we may use it to fix up relative OPT_FILENAME() options. Some\n> builtins don't bother; let's make sure we do so consistently.\n> \n> There may not be any particular bugs fixed here; OPT_FILENAME is\n> actually pretty rare, and none of these commands use it directly.\n> However, this does future-proof us against somebody adding an option\n> that uses it and creating a subtle bug that only shows up when you're in\n> a subdirectory of the repository.\n> \n> In some cases, like hash-object and upload-pack, we don't specify\n> RUN_SETUP, so we know the prefix will always be empty. It's still worth\n> passing the variable along to keep the idiom consistent across all\n> builtins (and of course it protects us if they ever _did_ switch to\n> using RUN_SETUP).\n\nGood explanation that we _should_ be using these parameters. Forgetting\nto add these prefix uses would probably not be obvious if a patch added\nRUN_SETUP.\n\nThanks,\n-Stolee\n"},{"id":"375289","messageId":"f0d06858-038b-793e-b1fe-faedff7963ef@gmail.com","threadId":"51062","inReplyTo":"20190509213229.GN15837@sigill.intra.peff.net","subject":"Re: [PATCH 14/14] verify-commit: simplify parameters to run_gpg_verify()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-10T12:19:56Z","receivedAt":"2019-05-10T12:20:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/9/2019 5:32 PM, Jeff King wrote:\n> Instead, let's do our type check by loading the object via\n> parse_object(). That will attach the buffer to the struct so it can be\n> used later by check_commit_signature(). And it ensures that\n> lookup_commit() will return something sane.\n\nThis is a good idea.\n  \n> -static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned long size, unsigned flags)\n> +static int run_gpg_verify(struct commit *commit, unsigned flags)\n>  {\n>  \tstruct signature_check signature_check;\n>  \tint ret;\n>  \n>  \tmemset(&signature_check, 0, sizeof(signature_check));\n>  \n> -\tret = check_commit_signature(lookup_commit(the_repository, oid),\n> -\t\t\t\t     &signature_check);\n> +\tret = check_commit_signature(commit, &signature_check);\n>  \tprint_signature_buffer(&signature_check, flags);\n\nBonus drop of the_repository.\n\n>  \n>  \tsignature_check_clear(&signature_check);\n> @@ -38,26 +37,20 @@ static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned\n>  \n>  static int verify_commit(const char *name, unsigned flags)\n>  {\n> -\tenum object_type type;\n>  \tstruct object_id oid;\n> -\tchar *buf;\n> -\tunsigned long size;\n> -\tint ret;\n> +\tstruct object *obj;\n>  \n>  \tif (get_oid(name, &oid))\n>  \t\treturn error(\"commit '%s' not found.\", name);\n>  \n> -\tbuf = read_object_file(&oid, &type, &size);\n> -\tif (!buf)\n> +\tobj = parse_object(the_repository, &oid);\n\n...and it is back, but \"higher\" up. That's fine.\n\n> +\tif (!obj)\n>  \t\treturn error(\"%s: unable to read file.\", name);\n> -\tif (type != OBJ_COMMIT)\n> +\tif (obj->type != OBJ_COMMIT)\n>  \t\treturn error(\"%s: cannot verify a non-commit object of type %s.\",\n> -\t\t\t\tname, type_name(type));\n> -\n> -\tret = run_gpg_verify(&oid, buf, size, flags);\n> +\t\t\t\tname, type_name(obj->type));\n>  \n> -\tfree(buf);\n> -\treturn ret;\n> +\treturn run_gpg_verify((struct commit *)obj, flags);\n>  }\n\nYou pointed out that you thought this patch was subtle. I agree that\nthere is a slightly changed functionality, but it seems to be\nimproving a possibly wrong behavior.\n\nThanks,\n-Stolee\n"},{"id":"375290","messageId":"aaa4a296-ff24-84f6-d67a-19feef200a48@gmail.com","threadId":"51062","inReplyTo":"20190509212558.GA15438@sigill.intra.peff.net","subject":"Re: [PATCH 0/14] \"final\" batch of unused parameter cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-10T12:20:49Z","receivedAt":"2019-05-10T12:20:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/9/2019 5:25 PM, Jeff King wrote:\n> This is a continuation of my efforts to get us compiling with\n> -Wunused-parameter.  This round finishes most of the cleanups and fixes\n> I intend to do (though I have a handful of cleanup cases that I'm still\n> poking at to make sure they're not in fact bugs). After that, I have\n> some patches to annotate unused parameters we can't get rid of (e.g.,\n> for cases where we're conforming to a callback interface), and then we\n> can finally flip the warning on for developer-mode.\n\nThis series looks correct to me. I look forward to having the warning\non in developer mode!\n\nThanks,\n-Stolee\n"},{"id":"375363","messageId":"xmqqlfzbc4wo.fsf@gitster-ct.c.googlers.com","threadId":"51062","inReplyTo":"20190509212724.GA15837@sigill.intra.peff.net","subject":"Re: [PATCH 01/14] cmd_{read,write}_tree: rename \"unused\" variable that is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-13T05:14:15Z","receivedAt":"2019-05-13T05:14:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I kind of hate \"cmd_prefix\"; I was tempted to just call it \"prefix\" so\n> that all of the cmd_* functions were consistent, but I worried that it\n> really would get confused with the local variables (even if those\n> variables are renamed, as I do here).\n\nI tend to agree with your temptation to make things consisten,\nespecially because you already do the renaming of existing \"prefix\"\nto \"tree_prefix\".\n\nPerhaps in a few release cycles, these can be renamed again to be\nconsistent with everybody else, and I won't die if this were called\ncmd_prefix until then ;-)\n\n"}]}