{"thread":{"id":"58327","subject":"[PATCH 0/6] unused function parameter potpourri","startedAt":"2022-08-19T08:48:55Z","lastAt":"2022-08-26T13:15:47Z","messageCount":21,"participants":["Jeff King","Phillip Wood","Derrick Stolee","Elijah Newren","René Scharfe","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"461512","messageId":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","threadId":"58327","inReplyTo":null,"subject":"[PATCH 0/6] unused function parameter potpourri","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-19T08:48:43Z","receivedAt":"2022-08-19T08:48:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are a few small cleanups of unused function parameters. The first\nfive just drop the unused parameters. These are all trivially correct,\nsince otherwise the compiler would complain. But I tried to make sure\nthat dropping was the right thing in each (rather than it being a bug\nwhere the parameter should have been used).\n\nThe final one just uses the parameters for an assertion, following a\npattern we've used before.\n\nI'll try to cc the individual authors for each patch.\n\n [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()\n [2/6]: log-tree: drop unused commit param in remerge_diff()\n [3/6]: match_pathname(): drop unused \"flags\" parameter\n [4/6]: verify_one_sparse(): drop unused repository parameter\n [5/6]: reftable: drop unused parameter from reader_seek_linear()\n [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks\n\n attr.c | 2 +-\n builtin/reflog.c | 4 ++++\n cache-tree.c | 6 ++----\n dir.c | 6 ++----\n dir.h | 2 +-\n log-tree.c | 5 ++---\n reftable/reader.c | 6 +++---\n xdiff/xdiffi.c | 2 +-\n xdiff/xdiffi.h | 3 +--\n xdiff/xhistogram.c | 3 +--\n 10 files changed, 18 insertions(+), 21 deletions(-)\n\n-Peff\n"},{"id":"461513","messageId":"Yv9OpXIQ9dYMQJ4B@coredump.intra.peff.net","threadId":"58327","inReplyTo":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","subject":"[PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-19T08:49:41Z","receivedAt":"2022-08-19T08:49:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These are no longer used since 9df0fc3d57 (xdiff: fix a memory leak,\n2022-02-16), as the caller is expected to call xdl_prepare_env() itself.\nAfter that change the histogram code only examines the prepared\nxdfenv_t, not the original buffers.\n\nSigned-off-by: Jeff King \n---\n xdiff/xdiffi.c | 2 +-\n xdiff/xdiffi.h | 3 +--\n xdiff/xhistogram.c | 3 +--\n 3 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 53e803e6bc..8c64519eac 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -326,7 +326,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t}\n \n \tif (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {\n-\t\tres = xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n+\t\tres = xdl_do_histogram_diff(xpp, xe);\n \t\tgoto out;\n \t}\n \ndiff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h\nindex 8f1c7c8b04..9d988e0263 100644\n--- a/xdiff/xdiffi.h\n+++ b/xdiff/xdiffi.h\n@@ -58,7 +58,6 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t xdemitconf_t const *xecfg);\n int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t\txdfenv_t *env);\n-int xdl_do_histogram_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n-\t\txdfenv_t *env);\n+int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);\n \n #endif /* #if !defined(XDIFFI_H) */\ndiff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\nindex df909004c1..16a8fe2f3f 100644\n--- a/xdiff/xhistogram.c\n+++ b/xdiff/xhistogram.c\n@@ -362,8 +362,7 @@ static int histogram_diff(xpparam_t const *xpp, xdfenv_t *env,\n \treturn result;\n }\n \n-int xdl_do_histogram_diff(mmfile_t *file1, mmfile_t *file2,\n-\txpparam_t const *xpp, xdfenv_t *env)\n+int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env)\n {\n \treturn histogram_diff(xpp, env,\n \t\tenv->xdf1.dstart + 1, env->xdf1.dend - env->xdf1.dstart + 1,\n-- \n2.37.2.928.g0821088f4a\n\n"},{"id":"461514","messageId":"Yv9O2RK7ahmw5ge7@coredump.intra.peff.net","threadId":"58327","inReplyTo":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","subject":"[PATCH 2/6] log-tree: drop unused commit param in remerge_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-19T08:50:33Z","receivedAt":"2022-08-19T08:50:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function has never used its \"commit\" parameter since it was added\nin db757e8b8d (show, log: provide a --remerge-diff capability,\n2022-02-02).\n\nThis makes sense; we already have separate parameters for the parents\n(which lets us redo the merge) and the oid of the result tree (which we\ncan then diff against the remerge result).\n\nLet's drop the unused parameter in the name of clarity.\n\nSigned-off-by: Jeff King \n---\n log-tree.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex d0ac0a6327..82d9b5f650 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -956,8 +956,7 @@ static void cleanup_additional_headers(struct diff_options *o)\n \n static int do_remerge_diff(struct rev_info *opt,\n \t\t\t struct commit_list *parents,\n-\t\t\t struct object_id *oid,\n-\t\t\t struct commit *commit)\n+\t\t\t struct object_id *oid)\n {\n \tstruct merge_options o;\n \tstruct commit_list *bases;\n@@ -1052,7 +1051,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n \t\t\t\t\t\"for octopus merges.\\n\");\n \t\t\t\treturn 1;\n \t\t\t}\n-\t\t\treturn do_remerge_diff(opt, parents, oid, commit);\n+\t\t\treturn do_remerge_diff(opt, parents, oid);\n \t\t}\n \t\tif (opt->combine_merges)\n \t\t\treturn do_diff_combined(opt, commit);\n-- \n2.37.2.928.g0821088f4a\n\n"},{"id":"461515","messageId":"Yv9O7nSS+WMzm0I7@coredump.intra.peff.net","threadId":"58327","inReplyTo":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","subject":"[PATCH 3/6] match_pathname(): drop unused \"flags\" parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-19T08:50:54Z","receivedAt":"2022-08-19T08:51:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This field has not been used since the function was introduced in\nb559263216 (exclude: split pathname matching code into a separate\nfunction, 2012-10-15), though there was a brief period where it was\nerroneously used and then reverted in ed4958477b (dir: fix pattern\nmatching on dirs, 2021-09-24) and 5ceb663e92 (dir: fix\ndirectory-matching bug, 2021-11-02).\n\nIt's possible we'd eventually add a flag that makes it useful here, but\nthere are only a handful of callers. It would be easy to add back if\nnecessary, and in the meantime this makes the function interface less\nmisleading.\n\nSigned-off-by: Jeff King \n---\n attr.c | 2 +-\n dir.c | 6 ++----\n dir.h | 2 +-\n 3 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 21e4ad25ad..8a78dde69e 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1023,7 +1023,7 @@ static int path_matches(const char *pathname, int pathlen,\n \t}\n \treturn match_pathname(pathname, pathlen - isdir,\n \t\t\t base, baselen,\n-\t\t\t pattern, prefix, pat->patternlen, pat->flags);\n+\t\t\t pattern, prefix, pat->patternlen);\n }\n \n static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem);\ndiff --git a/dir.c b/dir.c\nindex d7cfb08e44..50eeb8b11e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1244,8 +1244,7 @@ int match_basename(const char *basename, int basenamelen,\n \n int match_pathname(const char *pathname, int pathlen,\n \t\t const char *base, int baselen,\n-\t\t const char *pattern, int prefix, int patternlen,\n-\t\t unsigned flags)\n+\t\t const char *pattern, int prefix, int patternlen)\n {\n \tconst char *name;\n \tint namelen;\n@@ -1347,8 +1346,7 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname\n \t\tif (match_pathname(pathname, pathlen,\n \t\t\t\t pattern->base,\n \t\t\t\t pattern->baselen ? pattern->baselen - 1 : 0,\n-\t\t\t\t exclude, prefix, pattern->patternlen,\n-\t\t\t\t pattern->flags)) {\n+\t\t\t\t exclude, prefix, pattern->patternlen)) {\n \t\t\tres = pattern;\n \t\t\tbreak;\n \t\t}\ndiff --git a/dir.h b/dir.h\nindex 7bc862030c..674747d93a 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -414,7 +414,7 @@ int match_basename(const char *, int,\n \t\t const char *, int, int, unsigned);\n int match_pathname(const char *, int,\n \t\t const char *, int,\n-\t\t const char *, int, int, unsigned);\n+\t\t const char *, int, int);\n \n struct path_pattern *last_matching_pattern(struct dir_struct *dir,\n \t\t\t\t\t struct index_state *istate,\n-- \n2.37.2.928.g0821088f4a\n\n"},{"id":"461516","messageId":"Yv9O+HDMLKglLcqY@coredump.intra.peff.net","threadId":"58327","inReplyTo":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","subject":"[PATCH 4/6] verify_one_sparse(): drop unused repository parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-19T08:51:04Z","receivedAt":"2022-08-19T08:51:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function has never used its repository parameter since it was\nintroduced in 9ad2d5ea71 (sparse-index: loose integration with\ncache_tree_verify(), 2021-03-30). As that commit notes, it may\neventually be extended further, and that might require looking at a\nrepository struct. But it would be easy to add it back later if\nnecessary. In the mean time, dropping it makes the code shorter and\nappease -Wunused-parameter.\n\nSigned-off-by: Jeff King \n---\n cache-tree.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 56db0b5026..c97111cccf 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,\n \treturn 0;\n }\n \n-static void verify_one_sparse(struct repository *r,\n-\t\t\t struct index_state *istate,\n-\t\t\t struct cache_tree *it,\n+static void verify_one_sparse(struct index_state *istate,\n \t\t\t struct strbuf *path,\n \t\t\t int pos)\n {\n@@ -910,7 +908,7 @@ static int verify_one(struct repository *r,\n \t\t\treturn 1;\n \n \t\tif (pos >= 0) {\n-\t\t\tverify_one_sparse(r, istate, it, path, pos);\n+\t\t\tverify_one_sparse(istate, path, pos);\n \t\t\treturn 0;\n \t\t}\n \n-- \n2.37.2.928.g0821088f4a\n\n"},{"id":"461517","messageId":"Yv9P4O3WCrR9f9o2@coredump.intra.peff.net","threadId":"58327","inReplyTo":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","subject":"[PATCH 5/6] reftable: drop unused parameter from reader_seek_linear()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-19T08:54:56Z","receivedAt":"2022-08-19T08:55:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The reader code passes around a \"struct reftable_reader\" context\nvariable. But the seek function doesn't need it; the table iterator we\nalready get is sufficient.\n\nSigned-off-by: Jeff King \n---\nOne could argue that this is a method of a reftable_reader following the\nusual C object-oriented naming conventions, and thus should retain its\nfirst parameter, even if it isn't directly used.\n\nIn that case, we can annotate this as unused (once we have the ability\nto do so, which will be a separate series). I have a mild preference for\nremoving it (hence this patch), since I think that makes the code more\nclear. I suppose one could also argue that it should be a method of the\ntable_iter: table_iter_seek_linear() or something. I don't think it\nmatters much, and my ulterior motive is appeasing -Wunused-parameters.\n\n reftable/reader.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 54b4025105..b4db23ce18 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -443,7 +443,7 @@ static int reader_start(struct reftable_reader *r, struct table_iter *ti,\n \treturn reader_table_iter_at(r, ti, off, typ);\n }\n \n-static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti,\n+static int reader_seek_linear(struct table_iter *ti,\n \t\t\t struct reftable_record *want)\n {\n \tstruct reftable_record rec =\n@@ -510,7 +510,7 @@ static int reader_seek_indexed(struct reftable_reader *r,\n \tif (err < 0)\n \t\tgoto done;\n \n-\terr = reader_seek_linear(r, &index_iter, &want_index);\n+\terr = reader_seek_linear(&index_iter, &want_index);\n \twhile (1) {\n \t\terr = table_iter_next(&index_iter, &index_result);\n \t\ttable_iter_block_done(&index_iter);\n@@ -570,7 +570,7 @@ static int reader_seek_internal(struct reftable_reader *r,\n \terr = reader_start(r, &ti, reftable_record_type(rec), 0);\n \tif (err < 0)\n \t\treturn err;\n-\terr = reader_seek_linear(r, &ti, rec);\n+\terr = reader_seek_linear(&ti, rec);\n \tif (err < 0)\n \t\treturn err;\n \telse {\n-- \n2.37.2.928.g0821088f4a\n\n"},{"id":"461518","messageId":"Yv9P5l6ku3ZyG6yR@coredump.intra.peff.net","threadId":"58327","inReplyTo":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","subject":"[PATCH 6/6] reflog: assert PARSE_OPT_NONEG in parse-options callbacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-19T08:55:02Z","receivedAt":"2022-08-19T08:55:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the spirit of 517fe807d6 (assert NOARG/NONEG behavior of\nparse-options callbacks, 2018-11-05), this asserts that our callbacks\nwere invoked using the right flags (since otherwise they'd segfault on\nthe NULL arg). Both cases are already correct here, so this is mostly\nabout annotating the functions, and appeasing -Wunused-parameters.\n\nSigned-off-by: Jeff King \n---\n builtin/reflog.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 4dd297dce8..8123956847 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -193,6 +193,8 @@ static int expire_unreachable_callback(const struct option *opt,\n {\n \tstruct cmd_reflog_expire_cb *cmd = opt->value;\n \n+\tBUG_ON_OPT_NEG(unset);\n+\n \tif (parse_expiry_date(arg, &cmd->expire_unreachable))\n \t\tdie(_(\"invalid timestamp '%s' given to '--%s'\"),\n \t\t arg, opt->long_name);\n@@ -207,6 +209,8 @@ static int expire_total_callback(const struct option *opt,\n {\n \tstruct cmd_reflog_expire_cb *cmd = opt->value;\n \n+\tBUG_ON_OPT_NEG(unset);\n+\n \tif (parse_expiry_date(arg, &cmd->expire_total))\n \t\tdie(_(\"invalid timestamp '%s' given to '--%s'\"),\n \t\t arg, opt->long_name);\n-- \n2.37.2.928.g0821088f4a\n"},{"id":"461549","messageId":"CAPoeCOa6BDsunamy7_GtaSy-gL_0r3kAwDJ7ffA_uiFUzhen9w@mail.gmail.com","threadId":"58327","inReplyTo":"Yv9OpXIQ9dYMQJ4B@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-19T13:32:13Z","receivedAt":"2022-08-19T13:32:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn Fri, 19 Aug 2022 at 09:49, Jeff King wrote:\n>\n> These are no longer used since 9df0fc3d57 (xdiff: fix a memory leak,\n> 2022-02-16), as the caller is expected to call xdl_prepare_env() itself.\n> After that change the histogram code only examines the prepared\n> xdfenv_t, not the original buffers.\n\nThanks, I seem to have a blind spot for unused parameters (I think\nthis is at least the third such fix from you for one of my commits),\nI'm really looking forward to having -Wunused-parameter enabled,\nthanks for working on it. Looking at the xpatience.c I think we can\nremove the mmfile_t parameters there as well, they are only end up\nbeing used because patience_diff() gets called recursively. I'm about\nto go off list for a week, but I can look at putting a patch together\nfor that when I get back unless you want to.\n\nBest Wishes\n\nPhillip\n\n>\n> Signed-off-by: Jeff King \n> ---\n> xdiff/xdiffi.c | 2 +-\n> xdiff/xdiffi.h | 3 +--\n> xdiff/xhistogram.c | 3 +--\n> 3 files changed, 3 insertions(+), 5 deletions(-)\n>\n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index 53e803e6bc..8c64519eac 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -326,7 +326,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> }\n>\n> if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {\n> - res = xdl_do_histogram_diff(mf1, mf2, xpp, xe);\n> + res = xdl_do_histogram_diff(xpp, xe);\n> goto out;\n> }\n>\n> diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h\n> index 8f1c7c8b04..9d988e0263 100644\n> --- a/xdiff/xdiffi.h\n> +++ b/xdiff/xdiffi.h\n> @@ -58,7 +58,6 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n> xdemitconf_t const *xecfg);\n> int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> xdfenv_t *env);\n> -int xdl_do_histogram_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> - xdfenv_t *env);\n> +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);\n>\n> #endif /* #if !defined(XDIFFI_H) */\n> diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c\n> index df909004c1..16a8fe2f3f 100644\n> --- a/xdiff/xhistogram.c\n> +++ b/xdiff/xhistogram.c\n> @@ -362,8 +362,7 @@ static int histogram_diff(xpparam_t const *xpp, xdfenv_t *env,\n> return result;\n> }\n>\n> -int xdl_do_histogram_diff(mmfile_t *file1, mmfile_t *file2,\n> - xpparam_t const *xpp, xdfenv_t *env)\n> +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env)\n> {\n> return histogram_diff(xpp, env,\n> env->xdf1.dstart + 1, env->xdf1.dend - env->xdf1.dstart + 1,\n> --\n> 2.37.2.928.g0821088f4a\n>\n"},{"id":"461606","messageId":"cbbfbbdd-94de-d981-804b-fe199ca49754@github.com","threadId":"58327","inReplyTo":"Yv9O+HDMLKglLcqY@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-19T19:04:19Z","receivedAt":"2022-08-19T19:04:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2022 4:51 AM, Jeff King wrote:\n> This function has never used its repository parameter since it was\n> introduced in 9ad2d5ea71 (sparse-index: loose integration with\n> cache_tree_verify(), 2021-03-30). As that commit notes, it may\n> eventually be extended further, and that might require looking at a\n> repository struct. But it would be easy to add it back later if\n> necessary.\n\nThe good news is that 'struct index_state' now has a pointer to\nits repository, so we wouldn't need to add it back in.\n\nThanks,\n-Stolee\n\n"},{"id":"461608","messageId":"5f10be7e-424a-ed3b-bb77-75f1e026b2ae@github.com","threadId":"58327","inReplyTo":"Yv9P4O3WCrR9f9o2@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-19T19:06:23Z","receivedAt":"2022-08-19T19:06:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2022 4:54 AM, Jeff King wrote:\n> The reader code passes around a \"struct reftable_reader\" context\n> variable. But the seek function doesn't need it; the table iterator we\n> already get is sufficient.\n> \n> Signed-off-by: Jeff King \n> ---\n> One could argue that this is a method of a reftable_reader following the\n> usual C object-oriented naming conventions, and thus should retain its\n> first parameter, even if it isn't directly used.\n\n> -static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti,\n> +static int reader_seek_linear(struct table_iter *ti,\n> \t\t\t struct reftable_record *want)\n\nIf we wanted to make it seem more like it was a method of something,\nit now looks to operate on the table_iter, so the method name could\nchange to something like \"table_seek_linear()\" which might satisfy\nboth goals.\n\nThanks,\n-Stolee\n"},{"id":"461609","messageId":"892b2a45-1b6b-aa87-4902-471793135183@github.com","threadId":"58327","inReplyTo":"Yv9Oay+tNqhLDqVl@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] unused function parameter potpourri","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-19T19:08:03Z","receivedAt":"2022-08-19T19:08:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2022 4:48 AM, Jeff King wrote:\n> Here are a few small cleanups of unused function parameters. The first\n> five just drop the unused parameters. These are all trivially correct,\n> since otherwise the compiler would complain. But I tried to make sure\n> that dropping was the right thing in each (rather than it being a bug\n> where the parameter should have been used).\n> \n> The final one just uses the parameters for an assertion, following a\n> pattern we've used before.\n> \n> I'll try to cc the individual authors for each patch.\n> \n> [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()\n> [2/6]: log-tree: drop unused commit param in remerge_diff()\n> [3/6]: match_pathname(): drop unused \"flags\" parameter\n> [4/6]: verify_one_sparse(): drop unused repository parameter\n> [5/6]: reftable: drop unused parameter from reader_seek_linear()\n> [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks\n\nThanks for doing this cleanup. It all looks correct to me.\n\nPatch 5 mentioned some choice as to modifying the parameter list\nversus using the UNUSED() macro. I think renaming the method can\nsolve the uncertainty there, but it's also not necessary for the\nchange to be correct.\n\nThanks,\n-Stolee\n"},{"id":"461635","messageId":"CABPp-BHvscxV+vVL-Tew2H4h8V_3bZpD0Qz9uEMwrV=X3zrYSg@mail.gmail.com","threadId":"58327","inReplyTo":"Yv9O2RK7ahmw5ge7@coredump.intra.peff.net","subject":"Re: [PATCH 2/6] log-tree: drop unused commit param in remerge_diff()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-19T23:05:06Z","receivedAt":"2022-08-19T23:05:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 19, 2022 at 1:50 AM Jeff King wrote:\n>\n> This function has never used its \"commit\" parameter since it was added\n> in db757e8b8d (show, log: provide a --remerge-diff capability,\n> 2022-02-02).\n>\n> This makes sense; we already have separate parameters for the parents\n> (which lets us redo the merge) and the oid of the result tree (which we\n> can then diff against the remerge result).\n>\n> Let's drop the unused parameter in the name of clarity.\n>\n> Signed-off-by: Jeff King \n> ---\n> log-tree.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index d0ac0a6327..82d9b5f650 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -956,8 +956,7 @@ static void cleanup_additional_headers(struct diff_options *o)\n>\n> static int do_remerge_diff(struct rev_info *opt,\n> struct commit_list *parents,\n> - struct object_id *oid,\n> - struct commit *commit)\n> + struct object_id *oid)\n> {\n> struct merge_options o;\n> struct commit_list *bases;\n> @@ -1052,7 +1051,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n> \"for octopus merges.\\n\");\n> return 1;\n> }\n> - return do_remerge_diff(opt, parents, oid, commit);\n> + return do_remerge_diff(opt, parents, oid);\n> }\n> if (opt->combine_merges)\n> return do_diff_combined(opt, commit);\n> --\n> 2.37.2.928.g0821088f4a\n\nYeah, looks like I could have just used commit instead of parents and\noid, but since the calling code had those handy, I added them directly\nand forgot to remove commit.\n\nPatch looks good.\n"},{"id":"461636","messageId":"CABPp-BFoFBE4o1-M258-DHQSbW8WS2-EQCipCdmq2ECi9BL7pg@mail.gmail.com","threadId":"58327","inReplyTo":"892b2a45-1b6b-aa87-4902-471793135183@github.com","subject":"Re: [PATCH 0/6] unused function parameter potpourri","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-19T23:07:17Z","receivedAt":"2022-08-19T23:07:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 19, 2022 at 12:35 PM Derrick Stolee\n wrote:\n>\n> On 8/19/2022 4:48 AM, Jeff King wrote:\n> > Here are a few small cleanups of unused function parameters. The first\n> > five just drop the unused parameters. These are all trivially correct,\n> > since otherwise the compiler would complain. But I tried to make sure\n> > that dropping was the right thing in each (rather than it being a bug\n> > where the parameter should have been used).\n> >\n> > The final one just uses the parameters for an assertion, following a\n> > pattern we've used before.\n> >\n> > I'll try to cc the individual authors for each patch.\n> >\n> > [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()\n> > [2/6]: log-tree: drop unused commit param in remerge_diff()\n> > [3/6]: match_pathname(): drop unused \"flags\" parameter\n> > [4/6]: verify_one_sparse(): drop unused repository parameter\n> > [5/6]: reftable: drop unused parameter from reader_seek_linear()\n> > [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks\n>\n> Thanks for doing this cleanup. It all looks correct to me.\n\nLike Phillip, I seem to also have a blind spot for unused parameters\n-- this isn't the first one I created either.\n\nAnyway, this series looks good to me as well.\n\n> Patch 5 mentioned some choice as to modifying the parameter list\n> versus using the UNUSED() macro. I think renaming the method can\n> solve the uncertainty there, but it's also not necessary for the\n> change to be correct.\n"},{"id":"461644","messageId":"YwCGJLJU2+ui+hvn@coredump.intra.peff.net","threadId":"58327","inReplyTo":"CAPoeCOa6BDsunamy7_GtaSy-gL_0r3kAwDJ7ffA_uiFUzhen9w@mail.gmail.com","subject":"Re: [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-20T06:58:44Z","receivedAt":"2022-08-20T06:58:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2022 at 02:32:13PM +0100, Phillip Wood wrote:\n\n> Thanks, I seem to have a blind spot for unused parameters (I think\n> this is at least the third such fix from you for one of my commits),\n> I'm really looking forward to having -Wunused-parameter enabled,\n> thanks for working on it.\n\nI think everyone does. ;) I know I couldn't have found these without\nhelp from the compiler.\n\n> Looking at the xpatience.c I think we can remove the mmfile_t\n> parameters there as well, they are only end up being used because\n> patience_diff() gets called recursively. I'm about to go off list for\n> a week, but I can look at putting a patch together for that when I get\n> back unless you want to.\n\nI wondered that, too, but they also get passed in to fill_hashmap(),\nwhich records the pointers in its \"struct hashmap\". It looks like those\nfields are never accessed, though, and could even be removed from the\nstruct entirely!\n\nSo I think there is some room for cleanup.\n\n-Peff\n"},{"id":"461645","messageId":"YwCG1doiwSDOJuGK@coredump.intra.peff.net","threadId":"58327","inReplyTo":"cbbfbbdd-94de-d981-804b-fe199ca49754@github.com","subject":"Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-20T07:01:41Z","receivedAt":"2022-08-20T07:01:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2022 at 03:04:19PM -0400, Derrick Stolee wrote:\n\n> On 8/19/2022 4:51 AM, Jeff King wrote:\n> > This function has never used its repository parameter since it was\n> > introduced in 9ad2d5ea71 (sparse-index: loose integration with\n> > cache_tree_verify(), 2021-03-30). As that commit notes, it may\n> > eventually be extended further, and that might require looking at a\n> > repository struct. But it would be easy to add it back later if\n> > necessary.\n> \n> The good news is that 'struct index_state' now has a pointer to\n> its repository, so we wouldn't need to add it back in.\n\nOh, right, that is even better. I think I have a few other patches in\nthe pile that similarly remove repository pointers. When I'm writing\nthem up, I'll check to see if they can use the same (much better)\nreasoning.\n\n-Peff\n"},{"id":"461646","messageId":"YwCHhUGZvcRP9OQ2@coredump.intra.peff.net","threadId":"58327","inReplyTo":"CABPp-BHvscxV+vVL-Tew2H4h8V_3bZpD0Qz9uEMwrV=X3zrYSg@mail.gmail.com","subject":"Re: [PATCH 2/6] log-tree: drop unused commit param in remerge_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-20T07:04:37Z","receivedAt":"2022-08-20T07:04:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2022 at 04:05:06PM -0700, Elijah Newren wrote:\n\n> > This function has never used its \"commit\" parameter since it was added\n> > in db757e8b8d (show, log: provide a --remerge-diff capability,\n> > 2022-02-02).\n> >\n> > This makes sense; we already have separate parameters for the parents\n> > (which lets us redo the merge) and the oid of the result tree (which we\n> > can then diff against the remerge result).\n> [...]\n> \n> Yeah, looks like I could have just used commit instead of parents and\n> oid, but since the calling code had those handy, I added them directly\n> and forgot to remove commit.\n\nThat makes sense (the origin of these unused parameters are all\nmini-mysteries, so it's very satisfying to me to hear a plausible\nexplanation).\n\nI like using the broken-down parts as you ended up with. The sole caller\ndoes have a commit, but one doesn't _need_ to have a commit to do a\nremerge diff. You just need two tips and a proposed tree result. In\ntheory we could have a plumbing command which takes those individually.\n\n-Peff\n"},{"id":"461649","messageId":"YwCO+WE2Z+sJofEb@coredump.intra.peff.net","threadId":"58327","inReplyTo":"YwCGJLJU2+ui+hvn@coredump.intra.peff.net","subject":"[PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-20T07:36:25Z","receivedAt":"2022-08-20T07:36:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 20, 2022 at 02:58:44AM -0400, Jeff King wrote:\n\n> > Looking at the xpatience.c I think we can remove the mmfile_t\n> > parameters there as well, they are only end up being used because\n> > patience_diff() gets called recursively. I'm about to go off list for\n> > a week, but I can look at putting a patch together for that when I get\n> > back unless you want to.\n> \n> I wondered that, too, but they also get passed in to fill_hashmap(),\n> which records the pointers in its \"struct hashmap\". It looks like those\n> fields are never accessed, though, and could even be removed from the\n> struct entirely!\n> \n> So I think there is some room for cleanup.\n\nAnd here's a patch, so we don't forget about it. It could be applied\nindependently of this series, but there's a small textual dependency\nsince we're touching a line close to the related histogram cleanup.\n\nThanks for mentioning this. I had given only a cursory glance before,\nand just assumed it would be hard to trace. It turned out to be fun. :)\n\nJunio: this could go on top of the jk/unused-fixes topic.\n\n-- >8 --\nSubject: xdiff: drop unused mmfile parameters from xdl_do_patience_diff()\n\nThe entry point to the patience-diff algorithm takes two mmfile_t\nstructs with the original file contents, but it doesn't actually do\nanything useful with them. This is similar to the case recently cleaned\nup in the histogram code via f1d019071e (xdiff: drop unused mmfile\nparameters from xdl_do_histogram_diff(), 2022-08-19), but there's a bit\nmore subtlety going on.\n\nWe pass them into the recursive patience_diff(), which in turn passes\nthem into fill_hashmap(), which stuffs the pointers into a struct. But\nthe only thing which reads the struct fields is our recursion into\npatience_diff()!\n\nSo it's unlikely that something like -Wunused-parameter could find this\ncase: it would have to detect the circular dependency caused by the\nrecursion (not to mention tracing across struct field assignments).\n\nBut once found, it's easy to have the compiler confirm what's going on:\n\n 1. Drop the \"file1\" and \"file2\" fields from the hashmap struct\n definition. Remove the assignments in fill_hashmap(), and\n temporarily substitute NULL in the recursive call to\n patience_diff(). Compiling shows that no other code touched those\n fields.\n\n 2. Now fill_hashmap() will trigger -Wunused-parameter. Drop \"file1\"\n and \"file2\" from its definition and callsite.\n\n 3. Now patience_diff() will trigger -Wunused-parameter. Drop them\n there, too. One of the callsites is the recursion with our\n NULL values, so those temporary values go away.\n\n 4. Now xdl_do_patience_diff() will trigger -Wunused-parameter. Drop\n them there. And we're done.\n\nSuggested-by: Phillip Wood \nSigned-off-by: Jeff King \n---\nExplaining the 4 steps is probably overkill, but I found it a little\ninteresting how one can bend the compiler to their will here. At least I\ndidn't make it a series of 4 patches. ;)\n\n xdiff/xdiffi.c | 2 +-\n xdiff/xdiffi.h | 3 +--\n xdiff/xpatience.c | 23 +++++++----------------\n 3 files changed, 9 insertions(+), 19 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 8c64519eac..32652ded2d 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -321,7 +321,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \t\treturn -1;\n \n \tif (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {\n-\t\tres = xdl_do_patience_diff(mf1, mf2, xpp, xe);\n+\t\tres = xdl_do_patience_diff(xpp, xe);\n \t\tgoto out;\n \t}\n \ndiff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h\nindex 9d988e0263..126c9d8ff4 100644\n--- a/xdiff/xdiffi.h\n+++ b/xdiff/xdiffi.h\n@@ -56,8 +56,7 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr);\n void xdl_free_script(xdchange_t *xscr);\n int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t xdemitconf_t const *xecfg);\n-int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n-\t\txdfenv_t *env);\n+int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env);\n int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);\n \n #endif /* #if !defined(XDIFFI_H) */\ndiff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\nindex fe39c2978c..a2d8955537 100644\n--- a/xdiff/xpatience.c\n+++ b/xdiff/xpatience.c\n@@ -69,7 +69,6 @@ struct hashmap {\n \t} *entries, *first, *last;\n \t/* were common records found? */\n \tunsigned long has_matches;\n-\tmmfile_t *file1, *file2;\n \txdfenv_t *env;\n \txpparam_t const *xpp;\n };\n@@ -139,13 +138,10 @@ static void insert_record(xpparam_t const *xpp, int line, struct hashmap *map,\n *\n * It is assumed that env has been prepared using xdl_prepare().\n */\n-static int fill_hashmap(mmfile_t *file1, mmfile_t *file2,\n-\t\txpparam_t const *xpp, xdfenv_t *env,\n+static int fill_hashmap(xpparam_t const *xpp, xdfenv_t *env,\n \t\tstruct hashmap *result,\n \t\tint line1, int count1, int line2, int count2)\n {\n-\tresult->file1 = file1;\n-\tresult->file2 = file2;\n \tresult->xpp = xpp;\n \tresult->env = env;\n \n@@ -254,8 +250,7 @@ static int match(struct hashmap *map, int line1, int line2)\n \treturn record1->ha == record2->ha;\n }\n \n-static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n-\t\txpparam_t const *xpp, xdfenv_t *env,\n+static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,\n \t\tint line1, int count1, int line2, int count2);\n \n static int walk_common_sequence(struct hashmap *map, struct entry *first,\n@@ -286,8 +281,7 @@ static int walk_common_sequence(struct hashmap *map, struct entry *first,\n \n \t\t/* Recurse */\n \t\tif (next1 > line1 || next2 > line2) {\n-\t\t\tif (patience_diff(map->file1, map->file2,\n-\t\t\t\t\tmap->xpp, map->env,\n+\t\t\tif (patience_diff(map->xpp, map->env,\n \t\t\t\t\tline1, next1 - line1,\n \t\t\t\t\tline2, next2 - line2))\n \t\t\t\treturn -1;\n@@ -326,8 +320,7 @@ static int fall_back_to_classic_diff(struct hashmap *map,\n *\n * This function assumes that env was prepared with xdl_prepare_env().\n */\n-static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n-\t\txpparam_t const *xpp, xdfenv_t *env,\n+static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,\n \t\tint line1, int count1, int line2, int count2)\n {\n \tstruct hashmap map;\n@@ -346,7 +339,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n \t}\n \n \tmemset(&map, 0, sizeof(map));\n-\tif (fill_hashmap(file1, file2, xpp, env, &map,\n+\tif (fill_hashmap(xpp, env, &map,\n \t\t\tline1, count1, line2, count2))\n \t\treturn -1;\n \n@@ -374,9 +367,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n \treturn result;\n }\n \n-int xdl_do_patience_diff(mmfile_t *file1, mmfile_t *file2,\n-\t\txpparam_t const *xpp, xdfenv_t *env)\n+int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env)\n {\n-\treturn patience_diff(file1, file2, xpp, env,\n-\t\t\t1, env->xdf1.nrec, 1, env->xdf2.nrec);\n+\treturn patience_diff(xpp, env, 1, env->xdf1.nrec, 1, env->xdf2.nrec);\n }\n-- \n2.37.2.931.g60e3983abc\n\n"},{"id":"461651","messageId":"9829e1cf-21aa-0c10-0b64-c8a2ffdbc943@web.de","threadId":"58327","inReplyTo":"Yv9O+HDMLKglLcqY@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-08-20T08:48:47Z","receivedAt":"2022-08-20T08:49:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.08.22 um 10:51 schrieb Jeff King:\n> This function has never used its repository parameter since it was\n> introduced in 9ad2d5ea71 (sparse-index: loose integration with\n> cache_tree_verify(), 2021-03-30). As that commit notes, it may\n> eventually be extended further, and that might require looking at a\n> repository struct. But it would be easy to add it back later if\n> necessary. In the mean time, dropping it makes the code shorter and\n> appease -Wunused-parameter.\n>\n> Signed-off-by: Jeff King \n> ---\n> cache-tree.c | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 56db0b5026..c97111cccf 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,\n> \treturn 0;\n> }\n>\n> -static void verify_one_sparse(struct repository *r,\n> -\t\t\t struct index_state *istate,\n> -\t\t\t struct cache_tree *it,\n> +static void verify_one_sparse(struct index_state *istate,\n\nThis also removes the cache_tree parameter, which has never been used as\nwell, but is not mentioned in the commit message. A good change, to be\nsure.\n\n> \t\t\t struct strbuf *path,\n> \t\t\t int pos)\n> {\n> @@ -910,7 +908,7 @@ static int verify_one(struct repository *r,\n> \t\t\treturn 1;\n>\n> \t\tif (pos >= 0) {\n> -\t\t\tverify_one_sparse(r, istate, it, path, pos);\n> +\t\t\tverify_one_sparse(istate, path, pos);\n> \t\t\treturn 0;\n> \t\t}\n>\n\n"},{"id":"461653","messageId":"YwCjOETsh1o8u0Og@coredump.intra.peff.net","threadId":"58327","inReplyTo":"9829e1cf-21aa-0c10-0b64-c8a2ffdbc943@web.de","subject":"[PATCH v2 4/6] verify_one_sparse(): drop unused repository parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-20T09:02:48Z","receivedAt":"2022-08-20T09:02:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 20, 2022 at 10:48:47AM +0200, René Scharfe wrote:\n\n> > diff --git a/cache-tree.c b/cache-tree.c\n> > index 56db0b5026..c97111cccf 100644\n> > --- a/cache-tree.c\n> > +++ b/cache-tree.c\n> > @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,\n> > \treturn 0;\n> > }\n> >\n> > -static void verify_one_sparse(struct repository *r,\n> > -\t\t\t struct index_state *istate,\n> > -\t\t\t struct cache_tree *it,\n> > +static void verify_one_sparse(struct index_state *istate,\n> \n> This also removes the cache_tree parameter, which has never been used as\n> well, but is not mentioned in the commit message. A good change, to be\n> sure.\n\nGood catch. I wrote the patch itself over a year ago (whenever the\ncommit made it into 'next' and failed my compilation), but I didn't\nwrite the commit message until recently. And when re-reading the patch I\ntotally missed that _two_ parameters went away.\n\nGiven that and Stolee's earlier comment, here's a proposed commit\nmessage that explains it better:\n\n-- >8 --\nSubject: verify_one_sparse(): drop unused parameters\n\nThis function has never used its repository or cache_tree parameters\nsince it was introduced in 9ad2d5ea71 (sparse-index: loose integration\nwith cache_tree_verify(), 2021-03-30).\n\nAs that commit notes, it may eventually be extended further, and that\nmight require looking at more data. But we can easily add them back if\nnecessary (and the repository is even included in the index_state these\ndays already). In the mean time, dropping them makes the code shorter\nand appeases -Wunused-parameter.\n\nSigned-off-by: Jeff King \n---\n cache-tree.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 56db0b5026..c97111cccf 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,\n \treturn 0;\n }\n \n-static void verify_one_sparse(struct repository *r,\n-\t\t\t struct index_state *istate,\n-\t\t\t struct cache_tree *it,\n+static void verify_one_sparse(struct index_state *istate,\n \t\t\t struct strbuf *path,\n \t\t\t int pos)\n {\n@@ -910,7 +908,7 @@ static int verify_one(struct repository *r,\n \t\t\treturn 1;\n \n \t\tif (pos >= 0) {\n-\t\t\tverify_one_sparse(r, istate, it, path, pos);\n+\t\t\tverify_one_sparse(istate, path, pos);\n \t\t\treturn 0;\n \t\t}\n \n-- \n2.37.2.964.g6266ca593d\n\n"},{"id":"461746","messageId":"CAFQ2z_PHq6wFbZwiRnpS4NDmUu3nbRyhq=302Pys31mPa8VTew@mail.gmail.com","threadId":"58327","inReplyTo":"5f10be7e-424a-ed3b-bb77-75f1e026b2ae@github.com","subject":"Re: [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear()","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2022-08-22T07:46:11Z","receivedAt":"2022-08-22T07:46:27Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Aug 19, 2022 at 9:06 PM Derrick Stolee wrote:\n>\n> On 8/19/2022 4:54 AM, Jeff King wrote:\n> > The reader code passes around a \"struct reftable_reader\" context\n> > variable. But the seek function doesn't need it; the table iterator we\n> > already get is sufficient.\n>\n> If we wanted to make it seem more like it was a method of something,\n> it now looks to operate on the table_iter, so the method name could\n> change to something like \"table_seek_linear()\" which might satisfy\n> both goals.\n\n+1. Name should be table_iter_seek_linear to stick with naming\nconventions, and should move to the top of the file next to other\ntable_iter methods.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Liana Sebastian\n"},{"id":"461994","messageId":"CAPoeCOZFua+VvwBPix2M-8sRhfhymFCpejOPbCERtCp7fZ9FWQ@mail.gmail.com","threadId":"58327","inReplyTo":"YwCO+WE2Z+sJofEb@coredump.intra.peff.net","subject":"Re: [PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-08-26T13:15:20Z","receivedAt":"2022-08-26T13:15:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn Sat, 20 Aug 2022 at 08:36, Jeff King wrote:\n>\n> On Sat, Aug 20, 2022 at 02:58:44AM -0400, Jeff King wrote:\n>\n> > > Looking at the xpatience.c I think we can remove the mmfile_t\n> > > parameters there as well, they are only end up being used because\n> > > patience_diff() gets called recursively. I'm about to go off list for\n> > > a week, but I can look at putting a patch together for that when I get\n> > > back unless you want to.\n> >\n> > I wondered that, too, but they also get passed in to fill_hashmap(),\n> > which records the pointers in its \"struct hashmap\". It looks like those\n> > fields are never accessed, though, and could even be removed from the\n> > struct entirely!\n> >\n> > So I think there is some room for cleanup.\n>\n> And here's a patch, so we don't forget about it. It could be applied\n> independently of this series, but there's a small textual dependency\n> since we're touching a line close to the related histogram cleanup.\n>\n> Thanks for mentioning this. I had given only a cursory glance before,\n> and just assumed it would be hard to trace. It turned out to be fun. :)\n\nYes, it took me a couple of minutes to realize that it wasn't doing\nanything useful with the pointers in \"struct hashmap\" but it was quite\nsatisfying when I did. The patch looks good, thanks for putting it\ntogether.\n\nBest Wishes\n\nPhillip\n\n> Junio: this could go on top of the jk/unused-fixes topic.\n>\n> -- >8 --\n> Subject: xdiff: drop unused mmfile parameters from xdl_do_patience_diff()\n>\n> The entry point to the patience-diff algorithm takes two mmfile_t\n> structs with the original file contents, but it doesn't actually do\n> anything useful with them. This is similar to the case recently cleaned\n> up in the histogram code via f1d019071e (xdiff: drop unused mmfile\n> parameters from xdl_do_histogram_diff(), 2022-08-19), but there's a bit\n> more subtlety going on.\n>\n> We pass them into the recursive patience_diff(), which in turn passes\n> them into fill_hashmap(), which stuffs the pointers into a struct. But\n> the only thing which reads the struct fields is our recursion into\n> patience_diff()!\n>\n> So it's unlikely that something like -Wunused-parameter could find this\n> case: it would have to detect the circular dependency caused by the\n> recursion (not to mention tracing across struct field assignments).\n>\n> But once found, it's easy to have the compiler confirm what's going on:\n>\n> 1. Drop the \"file1\" and \"file2\" fields from the hashmap struct\n> definition. Remove the assignments in fill_hashmap(), and\n> temporarily substitute NULL in the recursive call to\n> patience_diff(). Compiling shows that no other code touched those\n> fields.\n>\n> 2. Now fill_hashmap() will trigger -Wunused-parameter. Drop \"file1\"\n> and \"file2\" from its definition and callsite.\n>\n> 3. Now patience_diff() will trigger -Wunused-parameter. Drop them\n> there, too. One of the callsites is the recursion with our\n> NULL values, so those temporary values go away.\n>\n> 4. Now xdl_do_patience_diff() will trigger -Wunused-parameter. Drop\n> them there. And we're done.\n>\n> Suggested-by: Phillip Wood \n> Signed-off-by: Jeff King \n> ---\n> Explaining the 4 steps is probably overkill, but I found it a little\n> interesting how one can bend the compiler to their will here. At least I\n> didn't make it a series of 4 patches. ;)\n>\n> xdiff/xdiffi.c | 2 +-\n> xdiff/xdiffi.h | 3 +--\n> xdiff/xpatience.c | 23 +++++++----------------\n> 3 files changed, 9 insertions(+), 19 deletions(-)\n>\n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index 8c64519eac..32652ded2d 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -321,7 +321,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> return -1;\n>\n> if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {\n> - res = xdl_do_patience_diff(mf1, mf2, xpp, xe);\n> + res = xdl_do_patience_diff(xpp, xe);\n> goto out;\n> }\n>\n> diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h\n> index 9d988e0263..126c9d8ff4 100644\n> --- a/xdiff/xdiffi.h\n> +++ b/xdiff/xdiffi.h\n> @@ -56,8 +56,7 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr);\n> void xdl_free_script(xdchange_t *xscr);\n> int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n> xdemitconf_t const *xecfg);\n> -int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n> - xdfenv_t *env);\n> +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env);\n> int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);\n>\n> #endif /* #if !defined(XDIFFI_H) */\n> diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c\n> index fe39c2978c..a2d8955537 100644\n> --- a/xdiff/xpatience.c\n> +++ b/xdiff/xpatience.c\n> @@ -69,7 +69,6 @@ struct hashmap {\n> } *entries, *first, *last;\n> /* were common records found? */\n> unsigned long has_matches;\n> - mmfile_t *file1, *file2;\n> xdfenv_t *env;\n> xpparam_t const *xpp;\n> };\n> @@ -139,13 +138,10 @@ static void insert_record(xpparam_t const *xpp, int line, struct hashmap *map,\n> *\n> * It is assumed that env has been prepared using xdl_prepare().\n> */\n> -static int fill_hashmap(mmfile_t *file1, mmfile_t *file2,\n> - xpparam_t const *xpp, xdfenv_t *env,\n> +static int fill_hashmap(xpparam_t const *xpp, xdfenv_t *env,\n> struct hashmap *result,\n> int line1, int count1, int line2, int count2)\n> {\n> - result->file1 = file1;\n> - result->file2 = file2;\n> result->xpp = xpp;\n> result->env = env;\n>\n> @@ -254,8 +250,7 @@ static int match(struct hashmap *map, int line1, int line2)\n> return record1->ha == record2->ha;\n> }\n>\n> -static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n> - xpparam_t const *xpp, xdfenv_t *env,\n> +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,\n> int line1, int count1, int line2, int count2);\n>\n> static int walk_common_sequence(struct hashmap *map, struct entry *first,\n> @@ -286,8 +281,7 @@ static int walk_common_sequence(struct hashmap *map, struct entry *first,\n>\n> /* Recurse */\n> if (next1 > line1 || next2 > line2) {\n> - if (patience_diff(map->file1, map->file2,\n> - map->xpp, map->env,\n> + if (patience_diff(map->xpp, map->env,\n> line1, next1 - line1,\n> line2, next2 - line2))\n> return -1;\n> @@ -326,8 +320,7 @@ static int fall_back_to_classic_diff(struct hashmap *map,\n> *\n> * This function assumes that env was prepared with xdl_prepare_env().\n> */\n> -static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n> - xpparam_t const *xpp, xdfenv_t *env,\n> +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,\n> int line1, int count1, int line2, int count2)\n> {\n> struct hashmap map;\n> @@ -346,7 +339,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n> }\n>\n> memset(&map, 0, sizeof(map));\n> - if (fill_hashmap(file1, file2, xpp, env, &map,\n> + if (fill_hashmap(xpp, env, &map,\n> line1, count1, line2, count2))\n> return -1;\n>\n> @@ -374,9 +367,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,\n> return result;\n> }\n>\n> -int xdl_do_patience_diff(mmfile_t *file1, mmfile_t *file2,\n> - xpparam_t const *xpp, xdfenv_t *env)\n> +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env)\n> {\n> - return patience_diff(file1, file2, xpp, env,\n> - 1, env->xdf1.nrec, 1, env->xdf2.nrec);\n> + return patience_diff(xpp, env, 1, env->xdf1.nrec, 1, env->xdf2.nrec);\n> }\n> --\n> 2.37.2.931.g60e3983abc\n>\n"}]}