[PATCH 0/6] unused function parameter potpourri

[PATCH 0/6] unused function parameter potpourri

[Jeff King]

Here are a few small cleanups of unused function parameters. The first
five just drop the unused parameters. These are all trivially correct,
since otherwise the compiler would complain. But I tried to make sure
that dropping was the right thing in each (rather than it being a bug
where the parameter should have been used).

The final one just uses the parameters for an assertion, following a
pattern we've used before.

I'll try to cc the individual authors for each patch.

  [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()
  [2/6]: log-tree: drop unused commit param in remerge_diff()
  [3/6]: match_pathname(): drop unused "flags" parameter
  [4/6]: verify_one_sparse(): drop unused repository parameter
  [5/6]: reftable: drop unused parameter from reader_seek_linear()
  [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks

 attr.c             | 2 +-
 builtin/reflog.c   | 4 ++++
 cache-tree.c       | 6 ++----
 dir.c              | 6 ++----
 dir.h              | 2 +-
 log-tree.c         | 5 ++---
 reftable/reader.c  | 6 +++---
 xdiff/xdiffi.c     | 2 +-
 xdiff/xdiffi.h     | 3 +--
 xdiff/xhistogram.c | 3 +--
 10 files changed, 18 insertions(+), 21 deletions(-)

-Peff

[PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()

[Jeff King]

These are no longer used since 9df0fc3d57 (xdiff: fix a memory leak,
2022-02-16), as the caller is expected to call xdl_prepare_env() itself.
After that change the histogram code only examines the prepared
xdfenv_t, not the original buffers.

Signed-off-by: Jeff King <peff@peff.net>
---
 xdiff/xdiffi.c     | 2 +-
 xdiff/xdiffi.h     | 3 +--
 xdiff/xhistogram.c | 3 +--
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 53e803e6bc..8c64519eac 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -326,7 +326,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 	}
 
 	if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {
-		res = xdl_do_histogram_diff(mf1, mf2, xpp, xe);
+		res = xdl_do_histogram_diff(xpp, xe);
 		goto out;
 	}
 
diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h
index 8f1c7c8b04..9d988e0263 100644
--- a/xdiff/xdiffi.h
+++ b/xdiff/xdiffi.h
@@ -58,7 +58,6 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		  xdemitconf_t const *xecfg);
 int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 		xdfenv_t *env);
-int xdl_do_histogram_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
-		xdfenv_t *env);
+int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);
 
 #endif /* #if !defined(XDIFFI_H) */
diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
index df909004c1..16a8fe2f3f 100644
--- a/xdiff/xhistogram.c
+++ b/xdiff/xhistogram.c
@@ -362,8 +362,7 @@ static int histogram_diff(xpparam_t const *xpp, xdfenv_t *env,
 	return result;
 }
 
-int xdl_do_histogram_diff(mmfile_t *file1, mmfile_t *file2,
-	xpparam_t const *xpp, xdfenv_t *env)
+int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env)
 {
 	return histogram_diff(xpp, env,
 		env->xdf1.dstart + 1, env->xdf1.dend - env->xdf1.dstart + 1,
-- 
2.37.2.928.g0821088f4a

[PATCH 2/6] log-tree: drop unused commit param in remerge_diff()

[Jeff King]

This function has never used its "commit" parameter since it was added
in db757e8b8d (show, log: provide a --remerge-diff capability,
2022-02-02).

This makes sense; we already have separate parameters for the parents
(which lets us redo the merge) and the oid of the result tree (which we
can then diff against the remerge result).

Let's drop the unused parameter in the name of clarity.

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

diff --git a/log-tree.c b/log-tree.c
index d0ac0a6327..82d9b5f650 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -956,8 +956,7 @@ static void cleanup_additional_headers(struct diff_options *o)
 
 static int do_remerge_diff(struct rev_info *opt,
 			   struct commit_list *parents,
-			   struct object_id *oid,
-			   struct commit *commit)
+			   struct object_id *oid)
 {
 	struct merge_options o;
 	struct commit_list *bases;
@@ -1052,7 +1051,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 					"for octopus merges.\n");
 				return 1;
 			}
-			return do_remerge_diff(opt, parents, oid, commit);
+			return do_remerge_diff(opt, parents, oid);
 		}
 		if (opt->combine_merges)
 			return do_diff_combined(opt, commit);
-- 
2.37.2.928.g0821088f4a

[PATCH 3/6] match_pathname(): drop unused "flags" parameter

[Jeff King]

This field has not been used since the function was introduced in
b559263216 (exclude: split pathname matching code into a separate
function, 2012-10-15), though there was a brief period where it was
erroneously used and then reverted in ed4958477b (dir: fix pattern
matching on dirs, 2021-09-24) and 5ceb663e92 (dir: fix
directory-matching bug, 2021-11-02).

It's possible we'd eventually add a flag that makes it useful here, but
there are only a handful of callers. It would be easy to add back if
necessary, and in the meantime this makes the function interface less
misleading.

Signed-off-by: Jeff King <peff@peff.net>
---
 attr.c | 2 +-
 dir.c  | 6 ++----
 dir.h  | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/attr.c b/attr.c
index 21e4ad25ad..8a78dde69e 100644
--- a/attr.c
+++ b/attr.c
@@ -1023,7 +1023,7 @@ static int path_matches(const char *pathname, int pathlen,
 	}
 	return match_pathname(pathname, pathlen - isdir,
 			      base, baselen,
-			      pattern, prefix, pat->patternlen, pat->flags);
+			      pattern, prefix, pat->patternlen);
 }
 
 static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem);
diff --git a/dir.c b/dir.c
index d7cfb08e44..50eeb8b11e 100644
--- a/dir.c
+++ b/dir.c
@@ -1244,8 +1244,7 @@ int match_basename(const char *basename, int basenamelen,
 
 int match_pathname(const char *pathname, int pathlen,
 		   const char *base, int baselen,
-		   const char *pattern, int prefix, int patternlen,
-		   unsigned flags)
+		   const char *pattern, int prefix, int patternlen)
 {
 	const char *name;
 	int namelen;
@@ -1347,8 +1346,7 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname
 		if (match_pathname(pathname, pathlen,
 				   pattern->base,
 				   pattern->baselen ? pattern->baselen - 1 : 0,
-				   exclude, prefix, pattern->patternlen,
-				   pattern->flags)) {
+				   exclude, prefix, pattern->patternlen)) {
 			res = pattern;
 			break;
 		}
diff --git a/dir.h b/dir.h
index 7bc862030c..674747d93a 100644
--- a/dir.h
+++ b/dir.h
@@ -414,7 +414,7 @@ int match_basename(const char *, int,
 		   const char *, int, int, unsigned);
 int match_pathname(const char *, int,
 		   const char *, int,
-		   const char *, int, int, unsigned);
+		   const char *, int, int);
 
 struct path_pattern *last_matching_pattern(struct dir_struct *dir,
 					   struct index_state *istate,
-- 
2.37.2.928.g0821088f4a

[PATCH 4/6] verify_one_sparse(): drop unused repository parameter

[Jeff King]

This function has never used its repository parameter since it was
introduced in 9ad2d5ea71 (sparse-index: loose integration with
cache_tree_verify(), 2021-03-30). As that commit notes, it may
eventually be extended further, and that might require looking at a
repository struct. But it would be easy to add it back later if
necessary. In the mean time, dropping it makes the code shorter and
appease -Wunused-parameter.

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

diff --git a/cache-tree.c b/cache-tree.c
index 56db0b5026..c97111cccf 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,
 	return 0;
 }
 
-static void verify_one_sparse(struct repository *r,
-			      struct index_state *istate,
-			      struct cache_tree *it,
+static void verify_one_sparse(struct index_state *istate,
 			      struct strbuf *path,
 			      int pos)
 {
@@ -910,7 +908,7 @@ static int verify_one(struct repository *r,
 			return 1;
 
 		if (pos >= 0) {
-			verify_one_sparse(r, istate, it, path, pos);
+			verify_one_sparse(istate, path, pos);
 			return 0;
 		}
 
-- 
2.37.2.928.g0821088f4a

[PATCH 5/6] reftable: drop unused parameter from reader_seek_linear()

[Jeff King]

The reader code passes around a "struct reftable_reader" context
variable. But the seek function doesn't need it; the table iterator we
already get is sufficient.

Signed-off-by: Jeff King <peff@peff.net>
---
One could argue that this is a method of a reftable_reader following the
usual C object-oriented naming conventions, and thus should retain its
first parameter, even if it isn't directly used.

In that case, we can annotate this as unused (once we have the ability
to do so, which will be a separate series). I have a mild preference for
removing it (hence this patch), since I think that makes the code more
clear. I suppose one could also argue that it should be a method of the
table_iter: table_iter_seek_linear() or something. I don't think it
matters much, and my ulterior motive is appeasing -Wunused-parameters.

 reftable/reader.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/reftable/reader.c b/reftable/reader.c
index 54b4025105..b4db23ce18 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -443,7 +443,7 @@ static int reader_start(struct reftable_reader *r, struct table_iter *ti,
 	return reader_table_iter_at(r, ti, off, typ);
 }
 
-static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti,
+static int reader_seek_linear(struct table_iter *ti,
 			      struct reftable_record *want)
 {
 	struct reftable_record rec =
@@ -510,7 +510,7 @@ static int reader_seek_indexed(struct reftable_reader *r,
 	if (err < 0)
 		goto done;
 
-	err = reader_seek_linear(r, &index_iter, &want_index);
+	err = reader_seek_linear(&index_iter, &want_index);
 	while (1) {
 		err = table_iter_next(&index_iter, &index_result);
 		table_iter_block_done(&index_iter);
@@ -570,7 +570,7 @@ static int reader_seek_internal(struct reftable_reader *r,
 	err = reader_start(r, &ti, reftable_record_type(rec), 0);
 	if (err < 0)
 		return err;
-	err = reader_seek_linear(r, &ti, rec);
+	err = reader_seek_linear(&ti, rec);
 	if (err < 0)
 		return err;
 	else {
-- 
2.37.2.928.g0821088f4a

[PATCH 6/6] reflog: assert PARSE_OPT_NONEG in parse-options callbacks

[Jeff King]

In the spirit of 517fe807d6 (assert NOARG/NONEG behavior of
parse-options callbacks, 2018-11-05), this asserts that our callbacks
were invoked using the right flags (since otherwise they'd segfault on
the NULL arg). Both cases are already correct here, so this is mostly
about annotating the functions, and appeasing -Wunused-parameters.

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

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4dd297dce8..8123956847 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -193,6 +193,8 @@ static int expire_unreachable_callback(const struct option *opt,
 {
 	struct cmd_reflog_expire_cb *cmd = opt->value;
 
+	BUG_ON_OPT_NEG(unset);
+
 	if (parse_expiry_date(arg, &cmd->expire_unreachable))
 		die(_("invalid timestamp '%s' given to '--%s'"),
 		    arg, opt->long_name);
@@ -207,6 +209,8 @@ static int expire_total_callback(const struct option *opt,
 {
 	struct cmd_reflog_expire_cb *cmd = opt->value;
 
+	BUG_ON_OPT_NEG(unset);
+
 	if (parse_expiry_date(arg, &cmd->expire_total))
 		die(_("invalid timestamp '%s' given to '--%s'"),
 		    arg, opt->long_name);
-- 
2.37.2.928.g0821088f4a

Re: [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()

[Phillip Wood]

Hi Peff

On Fri, 19 Aug 2022 at 09:49, Jeff King <peff@peff.net> wrote:
>
> These are no longer used since 9df0fc3d57 (xdiff: fix a memory leak,
> 2022-02-16), as the caller is expected to call xdl_prepare_env() itself.
> After that change the histogram code only examines the prepared
> xdfenv_t, not the original buffers.

Thanks, I seem to have a blind spot for unused parameters (I think
this is at least the third such fix from you for one of my commits),
I'm really looking forward to having -Wunused-parameter enabled,
thanks for working on it. Looking at the xpatience.c I think we can
remove the mmfile_t parameters there as well, they are only end up
being used because patience_diff() gets called recursively. I'm about
to go off list for a week, but I can look at putting a patch together
for that when I get back unless you want to.

Best Wishes

Phillip

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  xdiff/xdiffi.c     | 2 +-
>  xdiff/xdiffi.h     | 3 +--
>  xdiff/xhistogram.c | 3 +--
>  3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 53e803e6bc..8c64519eac 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -326,7 +326,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>         }
>
>         if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {
> -               res = xdl_do_histogram_diff(mf1, mf2, xpp, xe);
> +               res = xdl_do_histogram_diff(xpp, xe);
>                 goto out;
>         }
>
> diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h
> index 8f1c7c8b04..9d988e0263 100644
> --- a/xdiff/xdiffi.h
> +++ b/xdiff/xdiffi.h
> @@ -58,7 +58,6 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
>                   xdemitconf_t const *xecfg);
>  int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>                 xdfenv_t *env);
> -int xdl_do_histogram_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> -               xdfenv_t *env);
> +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);
>
>  #endif /* #if !defined(XDIFFI_H) */
> diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
> index df909004c1..16a8fe2f3f 100644
> --- a/xdiff/xhistogram.c
> +++ b/xdiff/xhistogram.c
> @@ -362,8 +362,7 @@ static int histogram_diff(xpparam_t const *xpp, xdfenv_t *env,
>         return result;
>  }
>
> -int xdl_do_histogram_diff(mmfile_t *file1, mmfile_t *file2,
> -       xpparam_t const *xpp, xdfenv_t *env)
> +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env)
>  {
>         return histogram_diff(xpp, env,
>                 env->xdf1.dstart + 1, env->xdf1.dend - env->xdf1.dstart + 1,
> --
> 2.37.2.928.g0821088f4a
>

Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter

[Derrick Stolee]

On 8/19/2022 4:51 AM, Jeff King wrote:
> This function has never used its repository parameter since it was
> introduced in 9ad2d5ea71 (sparse-index: loose integration with
> cache_tree_verify(), 2021-03-30). As that commit notes, it may
> eventually be extended further, and that might require looking at a
> repository struct. But it would be easy to add it back later if
> necessary.

The good news is that 'struct index_state' now has a pointer to
its repository, so we wouldn't need to add it back in.

Thanks,
-Stolee

Re: [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear()

[Derrick Stolee]

On 8/19/2022 4:54 AM, Jeff King wrote:
> The reader code passes around a "struct reftable_reader" context
> variable. But the seek function doesn't need it; the table iterator we
> already get is sufficient.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> One could argue that this is a method of a reftable_reader following the
> usual C object-oriented naming conventions, and thus should retain its
> first parameter, even if it isn't directly used.

> -static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti,
> +static int reader_seek_linear(struct table_iter *ti,
>  			      struct reftable_record *want)

If we wanted to make it seem more like it was a method of something,
it now looks to operate on the table_iter, so the method name could
change to something like "table_seek_linear()" which might satisfy
both goals.

Thanks,
-Stolee

Re: [PATCH 0/6] unused function parameter potpourri

[Derrick Stolee]

On 8/19/2022 4:48 AM, Jeff King wrote:
> Here are a few small cleanups of unused function parameters. The first
> five just drop the unused parameters. These are all trivially correct,
> since otherwise the compiler would complain. But I tried to make sure
> that dropping was the right thing in each (rather than it being a bug
> where the parameter should have been used).
> 
> The final one just uses the parameters for an assertion, following a
> pattern we've used before.
> 
> I'll try to cc the individual authors for each patch.
> 
>   [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()
>   [2/6]: log-tree: drop unused commit param in remerge_diff()
>   [3/6]: match_pathname(): drop unused "flags" parameter
>   [4/6]: verify_one_sparse(): drop unused repository parameter
>   [5/6]: reftable: drop unused parameter from reader_seek_linear()
>   [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks

Thanks for doing this cleanup. It all looks correct to me.

Patch 5 mentioned some choice as to modifying the parameter list
versus using the UNUSED() macro. I think renaming the method can
solve the uncertainty there, but it's also not necessary for the
change to be correct.

Thanks,
-Stolee

Re: [PATCH 2/6] log-tree: drop unused commit param in remerge_diff()

[Elijah Newren]

On Fri, Aug 19, 2022 at 1:50 AM Jeff King <peff@peff.net> wrote:
>
> This function has never used its "commit" parameter since it was added
> in db757e8b8d (show, log: provide a --remerge-diff capability,
> 2022-02-02).
>
> This makes sense; we already have separate parameters for the parents
> (which lets us redo the merge) and the oid of the result tree (which we
> can then diff against the remerge result).
>
> Let's drop the unused parameter in the name of clarity.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  log-tree.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index d0ac0a6327..82d9b5f650 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -956,8 +956,7 @@ static void cleanup_additional_headers(struct diff_options *o)
>
>  static int do_remerge_diff(struct rev_info *opt,
>                            struct commit_list *parents,
> -                          struct object_id *oid,
> -                          struct commit *commit)
> +                          struct object_id *oid)
>  {
>         struct merge_options o;
>         struct commit_list *bases;
> @@ -1052,7 +1051,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
>                                         "for octopus merges.\n");
>                                 return 1;
>                         }
> -                       return do_remerge_diff(opt, parents, oid, commit);
> +                       return do_remerge_diff(opt, parents, oid);
>                 }
>                 if (opt->combine_merges)
>                         return do_diff_combined(opt, commit);
> --
> 2.37.2.928.g0821088f4a

Yeah, looks like I could have just used commit instead of parents and
oid, but since the calling code had those handy, I added them directly
and forgot to remove commit.

Patch looks good.

Re: [PATCH 0/6] unused function parameter potpourri

[Elijah Newren]

On Fri, Aug 19, 2022 at 12:35 PM Derrick Stolee
<derrickstolee@github.com> wrote:
>
> On 8/19/2022 4:48 AM, Jeff King wrote:
> > Here are a few small cleanups of unused function parameters. The first
> > five just drop the unused parameters. These are all trivially correct,
> > since otherwise the compiler would complain. But I tried to make sure
> > that dropping was the right thing in each (rather than it being a bug
> > where the parameter should have been used).
> >
> > The final one just uses the parameters for an assertion, following a
> > pattern we've used before.
> >
> > I'll try to cc the individual authors for each patch.
> >
> >   [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()
> >   [2/6]: log-tree: drop unused commit param in remerge_diff()
> >   [3/6]: match_pathname(): drop unused "flags" parameter
> >   [4/6]: verify_one_sparse(): drop unused repository parameter
> >   [5/6]: reftable: drop unused parameter from reader_seek_linear()
> >   [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks
>
> Thanks for doing this cleanup. It all looks correct to me.

Like Phillip, I seem to also have a blind spot for unused parameters
-- this isn't the first one I created either.

Anyway, this series looks good to me as well.

> Patch 5 mentioned some choice as to modifying the parameter list
> versus using the UNUSED() macro. I think renaming the method can
> solve the uncertainty there, but it's also not necessary for the
> change to be correct.

Re: [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff()

[Jeff King]

On Fri, Aug 19, 2022 at 02:32:13PM +0100, Phillip Wood wrote:

> Thanks, I seem to have a blind spot for unused parameters (I think
> this is at least the third such fix from you for one of my commits),
> I'm really looking forward to having -Wunused-parameter enabled,
> thanks for working on it.

I think everyone does. ;) I know I couldn't have found these without
help from the compiler.

> Looking at the xpatience.c I think we can remove the mmfile_t
> parameters there as well, they are only end up being used because
> patience_diff() gets called recursively. I'm about to go off list for
> a week, but I can look at putting a patch together for that when I get
> back unless you want to.

I wondered that, too, but they also get passed in to fill_hashmap(),
which records the pointers in its "struct hashmap". It looks like those
fields are never accessed, though, and could even be removed from the
struct entirely!

So I think there is some room for cleanup.

-Peff

Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter

[Jeff King]

On Fri, Aug 19, 2022 at 03:04:19PM -0400, Derrick Stolee wrote:

> On 8/19/2022 4:51 AM, Jeff King wrote:
> > This function has never used its repository parameter since it was
> > introduced in 9ad2d5ea71 (sparse-index: loose integration with
> > cache_tree_verify(), 2021-03-30). As that commit notes, it may
> > eventually be extended further, and that might require looking at a
> > repository struct. But it would be easy to add it back later if
> > necessary.
> 
> The good news is that 'struct index_state' now has a pointer to
> its repository, so we wouldn't need to add it back in.

Oh, right, that is even better. I think I have a few other patches in
the pile that similarly remove repository pointers. When I'm writing
them up, I'll check to see if they can use the same (much better)
reasoning.

-Peff

Re: [PATCH 2/6] log-tree: drop unused commit param in remerge_diff()

[Jeff King]

On Fri, Aug 19, 2022 at 04:05:06PM -0700, Elijah Newren wrote:

> > This function has never used its "commit" parameter since it was added
> > in db757e8b8d (show, log: provide a --remerge-diff capability,
> > 2022-02-02).
> >
> > This makes sense; we already have separate parameters for the parents
> > (which lets us redo the merge) and the oid of the result tree (which we
> > can then diff against the remerge result).
> [...]
> 
> Yeah, looks like I could have just used commit instead of parents and
> oid, but since the calling code had those handy, I added them directly
> and forgot to remove commit.

That makes sense (the origin of these unused parameters are all
mini-mysteries, so it's very satisfying to me to hear a plausible
explanation).

I like using the broken-down parts as you ended up with. The sole caller
does have a commit, but one doesn't _need_ to have a commit to do a
remerge diff. You just need two tips and a proposed tree result. In
theory we could have a plumbing command which takes those individually.

-Peff

[PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff()

[Jeff King]

On Sat, Aug 20, 2022 at 02:58:44AM -0400, Jeff King wrote:

> > Looking at the xpatience.c I think we can remove the mmfile_t
> > parameters there as well, they are only end up being used because
> > patience_diff() gets called recursively. I'm about to go off list for
> > a week, but I can look at putting a patch together for that when I get
> > back unless you want to.
> 
> I wondered that, too, but they also get passed in to fill_hashmap(),
> which records the pointers in its "struct hashmap". It looks like those
> fields are never accessed, though, and could even be removed from the
> struct entirely!
> 
> So I think there is some room for cleanup.

And here's a patch, so we don't forget about it. It could be applied
independently of this series, but there's a small textual dependency
since we're touching a line close to the related histogram cleanup.

Thanks for mentioning this. I had given only a cursory glance before,
and just assumed it would be hard to trace. It turned out to be fun. :)

Junio: this could go on top of the jk/unused-fixes topic.

-- >8 --
Subject: xdiff: drop unused mmfile parameters from xdl_do_patience_diff()

The entry point to the patience-diff algorithm takes two mmfile_t
structs with the original file contents, but it doesn't actually do
anything useful with them. This is similar to the case recently cleaned
up in the histogram code via f1d019071e (xdiff: drop unused mmfile
parameters from xdl_do_histogram_diff(), 2022-08-19), but there's a bit
more subtlety going on.

We pass them into the recursive patience_diff(), which in turn passes
them into fill_hashmap(), which stuffs the pointers into a struct. But
the only thing which reads the struct fields is our recursion into
patience_diff()!

So it's unlikely that something like -Wunused-parameter could find this
case: it would have to detect the circular dependency caused by the
recursion (not to mention tracing across struct field assignments).

But once found, it's easy to have the compiler confirm what's going on:

  1. Drop the "file1" and "file2" fields from the hashmap struct
     definition. Remove the assignments in fill_hashmap(), and
     temporarily substitute NULL in the recursive call to
     patience_diff(). Compiling shows that no other code touched those
     fields.

  2. Now fill_hashmap() will trigger -Wunused-parameter. Drop "file1"
     and "file2" from its definition and callsite.

  3. Now patience_diff() will trigger -Wunused-parameter. Drop them
     there, too. One of the callsites is the recursion with our
     NULL values, so those temporary values go away.

  4. Now xdl_do_patience_diff() will trigger -Wunused-parameter. Drop
     them there. And we're done.

Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Jeff King <peff@peff.net>
---
Explaining the 4 steps is probably overkill, but I found it a little
interesting how one can bend the compiler to their will here. At least I
didn't make it a series of 4 patches. ;)

 xdiff/xdiffi.c    |  2 +-
 xdiff/xdiffi.h    |  3 +--
 xdiff/xpatience.c | 23 +++++++----------------
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 8c64519eac..32652ded2d 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -321,7 +321,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 		return -1;
 
 	if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
-		res = xdl_do_patience_diff(mf1, mf2, xpp, xe);
+		res = xdl_do_patience_diff(xpp, xe);
 		goto out;
 	}
 
diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h
index 9d988e0263..126c9d8ff4 100644
--- a/xdiff/xdiffi.h
+++ b/xdiff/xdiffi.h
@@ -56,8 +56,7 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr);
 void xdl_free_script(xdchange_t *xscr);
 int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		  xdemitconf_t const *xecfg);
-int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
-		xdfenv_t *env);
+int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env);
 int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);
 
 #endif /* #if !defined(XDIFFI_H) */
diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c
index fe39c2978c..a2d8955537 100644
--- a/xdiff/xpatience.c
+++ b/xdiff/xpatience.c
@@ -69,7 +69,6 @@ struct hashmap {
 	} *entries, *first, *last;
 	/* were common records found? */
 	unsigned long has_matches;
-	mmfile_t *file1, *file2;
 	xdfenv_t *env;
 	xpparam_t const *xpp;
 };
@@ -139,13 +138,10 @@ static void insert_record(xpparam_t const *xpp, int line, struct hashmap *map,
  *
  * It is assumed that env has been prepared using xdl_prepare().
  */
-static int fill_hashmap(mmfile_t *file1, mmfile_t *file2,
-		xpparam_t const *xpp, xdfenv_t *env,
+static int fill_hashmap(xpparam_t const *xpp, xdfenv_t *env,
 		struct hashmap *result,
 		int line1, int count1, int line2, int count2)
 {
-	result->file1 = file1;
-	result->file2 = file2;
 	result->xpp = xpp;
 	result->env = env;
 
@@ -254,8 +250,7 @@ static int match(struct hashmap *map, int line1, int line2)
 	return record1->ha == record2->ha;
 }
 
-static int patience_diff(mmfile_t *file1, mmfile_t *file2,
-		xpparam_t const *xpp, xdfenv_t *env,
+static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,
 		int line1, int count1, int line2, int count2);
 
 static int walk_common_sequence(struct hashmap *map, struct entry *first,
@@ -286,8 +281,7 @@ static int walk_common_sequence(struct hashmap *map, struct entry *first,
 
 		/* Recurse */
 		if (next1 > line1 || next2 > line2) {
-			if (patience_diff(map->file1, map->file2,
-					map->xpp, map->env,
+			if (patience_diff(map->xpp, map->env,
 					line1, next1 - line1,
 					line2, next2 - line2))
 				return -1;
@@ -326,8 +320,7 @@ static int fall_back_to_classic_diff(struct hashmap *map,
  *
  * This function assumes that env was prepared with xdl_prepare_env().
  */
-static int patience_diff(mmfile_t *file1, mmfile_t *file2,
-		xpparam_t const *xpp, xdfenv_t *env,
+static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,
 		int line1, int count1, int line2, int count2)
 {
 	struct hashmap map;
@@ -346,7 +339,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,
 	}
 
 	memset(&map, 0, sizeof(map));
-	if (fill_hashmap(file1, file2, xpp, env, &map,
+	if (fill_hashmap(xpp, env, &map,
 			line1, count1, line2, count2))
 		return -1;
 
@@ -374,9 +367,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,
 	return result;
 }
 
-int xdl_do_patience_diff(mmfile_t *file1, mmfile_t *file2,
-		xpparam_t const *xpp, xdfenv_t *env)
+int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env)
 {
-	return patience_diff(file1, file2, xpp, env,
-			1, env->xdf1.nrec, 1, env->xdf2.nrec);
+	return patience_diff(xpp, env, 1, env->xdf1.nrec, 1, env->xdf2.nrec);
 }
-- 
2.37.2.931.g60e3983abc

Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter

[René Scharfe]

Am 19.08.22 um 10:51 schrieb Jeff King:
> This function has never used its repository parameter since it was
> introduced in 9ad2d5ea71 (sparse-index: loose integration with
> cache_tree_verify(), 2021-03-30). As that commit notes, it may
> eventually be extended further, and that might require looking at a
> repository struct. But it would be easy to add it back later if
> necessary. In the mean time, dropping it makes the code shorter and
> appease -Wunused-parameter.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  cache-tree.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 56db0b5026..c97111cccf 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,
>  	return 0;
>  }
>
> -static void verify_one_sparse(struct repository *r,
> -			      struct index_state *istate,
> -			      struct cache_tree *it,
> +static void verify_one_sparse(struct index_state *istate,

This also removes the cache_tree parameter, which has never been used as
well, but is not mentioned in the commit message.  A good change, to be
sure.

>  			      struct strbuf *path,
>  			      int pos)
>  {
> @@ -910,7 +908,7 @@ static int verify_one(struct repository *r,
>  			return 1;
>
>  		if (pos >= 0) {
> -			verify_one_sparse(r, istate, it, path, pos);
> +			verify_one_sparse(istate, path, pos);
>  			return 0;
>  		}
>

[PATCH v2 4/6] verify_one_sparse(): drop unused repository parameter

[Jeff King]

On Sat, Aug 20, 2022 at 10:48:47AM +0200, René Scharfe wrote:

> > diff --git a/cache-tree.c b/cache-tree.c
> > index 56db0b5026..c97111cccf 100644
> > --- a/cache-tree.c
> > +++ b/cache-tree.c
> > @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,
> >  	return 0;
> >  }
> >
> > -static void verify_one_sparse(struct repository *r,
> > -			      struct index_state *istate,
> > -			      struct cache_tree *it,
> > +static void verify_one_sparse(struct index_state *istate,
> 
> This also removes the cache_tree parameter, which has never been used as
> well, but is not mentioned in the commit message.  A good change, to be
> sure.

Good catch. I wrote the patch itself over a year ago (whenever the
commit made it into 'next' and failed my compilation), but I didn't
write the commit message until recently. And when re-reading the patch I
totally missed that _two_ parameters went away.

Given that and Stolee's earlier comment, here's a proposed commit
message that explains it better:

-- >8 --
Subject: verify_one_sparse(): drop unused parameters

This function has never used its repository or cache_tree parameters
since it was introduced in 9ad2d5ea71 (sparse-index: loose integration
with cache_tree_verify(), 2021-03-30).

As that commit notes, it may eventually be extended further, and that
might require looking at more data. But we can easily add them back if
necessary (and the repository is even included in the index_state these
days already). In the mean time, dropping them makes the code shorter
and appeases -Wunused-parameter.

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

diff --git a/cache-tree.c b/cache-tree.c
index 56db0b5026..c97111cccf 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,
 	return 0;
 }
 
-static void verify_one_sparse(struct repository *r,
-			      struct index_state *istate,
-			      struct cache_tree *it,
+static void verify_one_sparse(struct index_state *istate,
 			      struct strbuf *path,
 			      int pos)
 {
@@ -910,7 +908,7 @@ static int verify_one(struct repository *r,
 			return 1;
 
 		if (pos >= 0) {
-			verify_one_sparse(r, istate, it, path, pos);
+			verify_one_sparse(istate, path, pos);
 			return 0;
 		}
 
-- 
2.37.2.964.g6266ca593d

Re: [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear()

[Han-Wen Nienhuys]

On Fri, Aug 19, 2022 at 9:06 PM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 8/19/2022 4:54 AM, Jeff King wrote:
> > The reader code passes around a "struct reftable_reader" context
> > variable. But the seek function doesn't need it; the table iterator we
> > already get is sufficient.
>
> If we wanted to make it seem more like it was a method of something,
> it now looks to operate on the table_iter, so the method name could
> change to something like "table_seek_linear()" which might satisfy
> both goals.

+1. Name should be table_iter_seek_linear to stick with naming
conventions, and should move to the top of the file next to other
table_iter methods.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Liana Sebastian

Re: [PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff()

[Phillip Wood]

Hi Peff

On Sat, 20 Aug 2022 at 08:36, Jeff King <peff@peff.net> wrote:
>
> On Sat, Aug 20, 2022 at 02:58:44AM -0400, Jeff King wrote:
>
> > > Looking at the xpatience.c I think we can remove the mmfile_t
> > > parameters there as well, they are only end up being used because
> > > patience_diff() gets called recursively. I'm about to go off list for
> > > a week, but I can look at putting a patch together for that when I get
> > > back unless you want to.
> >
> > I wondered that, too, but they also get passed in to fill_hashmap(),
> > which records the pointers in its "struct hashmap". It looks like those
> > fields are never accessed, though, and could even be removed from the
> > struct entirely!
> >
> > So I think there is some room for cleanup.
>
> And here's a patch, so we don't forget about it. It could be applied
> independently of this series, but there's a small textual dependency
> since we're touching a line close to the related histogram cleanup.
>
> Thanks for mentioning this. I had given only a cursory glance before,
> and just assumed it would be hard to trace. It turned out to be fun. :)

Yes, it took me a couple of minutes to realize that it wasn't doing
anything useful with the pointers in "struct hashmap" but it was quite
satisfying when I did. The patch looks good, thanks for putting it
together.

Best Wishes

Phillip

> Junio: this could go on top of the jk/unused-fixes topic.
>
> -- >8 --
> Subject: xdiff: drop unused mmfile parameters from xdl_do_patience_diff()
>
> The entry point to the patience-diff algorithm takes two mmfile_t
> structs with the original file contents, but it doesn't actually do
> anything useful with them. This is similar to the case recently cleaned
> up in the histogram code via f1d019071e (xdiff: drop unused mmfile
> parameters from xdl_do_histogram_diff(), 2022-08-19), but there's a bit
> more subtlety going on.
>
> We pass them into the recursive patience_diff(), which in turn passes
> them into fill_hashmap(), which stuffs the pointers into a struct. But
> the only thing which reads the struct fields is our recursion into
> patience_diff()!
>
> So it's unlikely that something like -Wunused-parameter could find this
> case: it would have to detect the circular dependency caused by the
> recursion (not to mention tracing across struct field assignments).
>
> But once found, it's easy to have the compiler confirm what's going on:
>
>   1. Drop the "file1" and "file2" fields from the hashmap struct
>      definition. Remove the assignments in fill_hashmap(), and
>      temporarily substitute NULL in the recursive call to
>      patience_diff(). Compiling shows that no other code touched those
>      fields.
>
>   2. Now fill_hashmap() will trigger -Wunused-parameter. Drop "file1"
>      and "file2" from its definition and callsite.
>
>   3. Now patience_diff() will trigger -Wunused-parameter. Drop them
>      there, too. One of the callsites is the recursion with our
>      NULL values, so those temporary values go away.
>
>   4. Now xdl_do_patience_diff() will trigger -Wunused-parameter. Drop
>      them there. And we're done.
>
> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Explaining the 4 steps is probably overkill, but I found it a little
> interesting how one can bend the compiler to their will here. At least I
> didn't make it a series of 4 patches. ;)
>
>  xdiff/xdiffi.c    |  2 +-
>  xdiff/xdiffi.h    |  3 +--
>  xdiff/xpatience.c | 23 +++++++----------------
>  3 files changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 8c64519eac..32652ded2d 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -321,7 +321,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>                 return -1;
>
>         if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
> -               res = xdl_do_patience_diff(mf1, mf2, xpp, xe);
> +               res = xdl_do_patience_diff(xpp, xe);
>                 goto out;
>         }
>
> diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h
> index 9d988e0263..126c9d8ff4 100644
> --- a/xdiff/xdiffi.h
> +++ b/xdiff/xdiffi.h
> @@ -56,8 +56,7 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr);
>  void xdl_free_script(xdchange_t *xscr);
>  int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
>                   xdemitconf_t const *xecfg);
> -int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> -               xdfenv_t *env);
> +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env);
>  int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env);
>
>  #endif /* #if !defined(XDIFFI_H) */
> diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c
> index fe39c2978c..a2d8955537 100644
> --- a/xdiff/xpatience.c
> +++ b/xdiff/xpatience.c
> @@ -69,7 +69,6 @@ struct hashmap {
>         } *entries, *first, *last;
>         /* were common records found? */
>         unsigned long has_matches;
> -       mmfile_t *file1, *file2;
>         xdfenv_t *env;
>         xpparam_t const *xpp;
>  };
> @@ -139,13 +138,10 @@ static void insert_record(xpparam_t const *xpp, int line, struct hashmap *map,
>   *
>   * It is assumed that env has been prepared using xdl_prepare().
>   */
> -static int fill_hashmap(mmfile_t *file1, mmfile_t *file2,
> -               xpparam_t const *xpp, xdfenv_t *env,
> +static int fill_hashmap(xpparam_t const *xpp, xdfenv_t *env,
>                 struct hashmap *result,
>                 int line1, int count1, int line2, int count2)
>  {
> -       result->file1 = file1;
> -       result->file2 = file2;
>         result->xpp = xpp;
>         result->env = env;
>
> @@ -254,8 +250,7 @@ static int match(struct hashmap *map, int line1, int line2)
>         return record1->ha == record2->ha;
>  }
>
> -static int patience_diff(mmfile_t *file1, mmfile_t *file2,
> -               xpparam_t const *xpp, xdfenv_t *env,
> +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,
>                 int line1, int count1, int line2, int count2);
>
>  static int walk_common_sequence(struct hashmap *map, struct entry *first,
> @@ -286,8 +281,7 @@ static int walk_common_sequence(struct hashmap *map, struct entry *first,
>
>                 /* Recurse */
>                 if (next1 > line1 || next2 > line2) {
> -                       if (patience_diff(map->file1, map->file2,
> -                                       map->xpp, map->env,
> +                       if (patience_diff(map->xpp, map->env,
>                                         line1, next1 - line1,
>                                         line2, next2 - line2))
>                                 return -1;
> @@ -326,8 +320,7 @@ static int fall_back_to_classic_diff(struct hashmap *map,
>   *
>   * This function assumes that env was prepared with xdl_prepare_env().
>   */
> -static int patience_diff(mmfile_t *file1, mmfile_t *file2,
> -               xpparam_t const *xpp, xdfenv_t *env,
> +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env,
>                 int line1, int count1, int line2, int count2)
>  {
>         struct hashmap map;
> @@ -346,7 +339,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,
>         }
>
>         memset(&map, 0, sizeof(map));
> -       if (fill_hashmap(file1, file2, xpp, env, &map,
> +       if (fill_hashmap(xpp, env, &map,
>                         line1, count1, line2, count2))
>                 return -1;
>
> @@ -374,9 +367,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2,
>         return result;
>  }
>
> -int xdl_do_patience_diff(mmfile_t *file1, mmfile_t *file2,
> -               xpparam_t const *xpp, xdfenv_t *env)
> +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env)
>  {
> -       return patience_diff(file1, file2, xpp, env,
> -                       1, env->xdf1.nrec, 1, env->xdf2.nrec);
> +       return patience_diff(xpp, env, 1, env->xdf1.nrec, 1, env->xdf2.nrec);
>  }
> --
> 2.37.2.931.g60e3983abc
>