[PATCH 0/9] drop some unused parameters

[PATCH 0/9] drop some unused parameters

[Jeff King]

I've mentioned before that I have a series which compiles cleanly with
-Wunused-parameters. I split this work roughly into three groups:

  1. Patches that fix bugs (i.e., we should have been using the
     parameter but didn't).

  2. Patches that drop unused parameters (i.e., code cleanup).

  3. Patches that annotate undroppable parameters (e.g., ones that are
     present due to a callback interface).

All of the patches from group 1 have been merged already. So this series
starts us off on group 2. There are about 50 patches in that group.
Given that none of them are urgent, I plan to feed them in batches to
avoid overwhelming reviewers. I'm also ordering them to avoid conflicts
with other topics in flight (this batch has no conflicts with 'next',
and only one minor textual conflict with 'pu').

The patches themselves are pretty much independent from each other. I
based these on master. They're cleanup which _could_ go to maint, and I
suspect they'd apply there, too (most of these are pretty old), but
again, I don't think there's a particular urgency to this.

  [1/8]: match-trees: drop unused path parameter from score functions
  [2/8]: apply: drop unused "def" parameter from find_name_gnu()
  [3/8]: create_bundle(): drop unused "header" parameter
  [4/8]: column: drop unused "opts" parameter in item_length()
  [5/8]: show_date_relative(): drop unused "tz" parameter
  [6/8]: config: drop unused parameter from maybe_remove_section()
  [7/8]: convert: drop len parameter from conversion checks
  [8/8]: convert: drop path parameter from actual conversion functions

 apply.c              |  5 ++---
 builtin/bundle.c     |  3 +--
 bundle.c             |  4 ++--
 bundle.h             |  4 ++--
 cache.h              |  2 +-
 column.c             |  4 ++--
 config.c             |  3 +--
 convert.c            | 28 ++++++++++++++--------------
 date.c               |  8 ++++----
 match-trees.c        | 16 +++++++---------
 t/helper/test-date.c |  2 +-
 11 files changed, 37 insertions(+), 42 deletions(-)

[PATCH 1/8] match-trees: drop unused path parameter from score functions

[Jeff King]

The scores do not take the particular path into account at
all. It's possible they could, but these are all static file-local
functions. It won't be a big deal to re-add the parameter if they ever
need it.

Signed-off-by: Jeff King <peff@peff.net>
---
 match-trees.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 2b6d31ef9d..e65e665bf5 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -3,7 +3,7 @@
 #include "tree-walk.h"
 #include "object-store.h"
 
-static int score_missing(unsigned mode, const char *path)
+static int score_missing(unsigned mode)
 {
 	int score;
 
@@ -16,7 +16,7 @@ static int score_missing(unsigned mode, const char *path)
 	return score;
 }
 
-static int score_differs(unsigned mode1, unsigned mode2, const char *path)
+static int score_differs(unsigned mode1, unsigned mode2)
 {
 	int score;
 
@@ -29,7 +29,7 @@ static int score_differs(unsigned mode1, unsigned mode2, const char *path)
 	return score;
 }
 
-static int score_matches(unsigned mode1, unsigned mode2, const char *path)
+static int score_matches(unsigned mode1, unsigned mode2)
 {
 	int score;
 
@@ -98,24 +98,22 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha
 
 		if (cmp < 0) {
 			/* path1 does not appear in two */
-			score += score_missing(one.entry.mode, one.entry.path);
+			score += score_missing(one.entry.mode);
 			update_tree_entry(&one);
 		} else if (cmp > 0) {
 			/* path2 does not appear in one */
-			score += score_missing(two.entry.mode, two.entry.path);
+			score += score_missing(two.entry.mode);
 			update_tree_entry(&two);
 		} else {
 			/* path appears in both */
 			if (!oideq(one.entry.oid, two.entry.oid)) {
 				/* they are different */
 				score += score_differs(one.entry.mode,
-						       two.entry.mode,
-						       one.entry.path);
+						       two.entry.mode);
 			} else {
 				/* same subtree or blob */
 				score += score_matches(one.entry.mode,
-						       two.entry.mode,
-						       one.entry.path);
+						       two.entry.mode);
 			}
 			update_tree_entry(&one);
 			update_tree_entry(&two);
-- 
2.20.1.842.g8986705066

[PATCH 2/8] apply: drop unused "def" parameter from find_name_gnu()

[Jeff King]

We use the "def" parameter in find_name_common() for some heuristics,
but they are not necessary with the less-ambiguous GNU format. Let's
drop this unused parameter.

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

diff --git a/apply.c b/apply.c
index 3703bfc8d0..ccab7e7907 100644
--- a/apply.c
+++ b/apply.c
@@ -467,7 +467,6 @@ static char *squash_slash(char *name)
 
 static char *find_name_gnu(struct apply_state *state,
 			   const char *line,
-			   const char *def,
 			   int p_value)
 {
 	struct strbuf name = STRBUF_INIT;
@@ -714,7 +713,7 @@ static char *find_name(struct apply_state *state,
 		       int terminate)
 {
 	if (*line == '"') {
-		char *name = find_name_gnu(state, line, def, p_value);
+		char *name = find_name_gnu(state, line, p_value);
 		if (name)
 			return name;
 	}
@@ -731,7 +730,7 @@ static char *find_name_traditional(struct apply_state *state,
 	size_t date_len;
 
 	if (*line == '"') {
-		char *name = find_name_gnu(state, line, def, p_value);
+		char *name = find_name_gnu(state, line, p_value);
 		if (name)
 			return name;
 	}
-- 
2.20.1.842.g8986705066

[PATCH 3/8] create_bundle(): drop unused "header" parameter

[Jeff King]

There's no need to pass a header struct to create_bundle(); it writes
the header information directly to a descriptor (and does not report
back details to the caller).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/bundle.c | 3 +--
 bundle.c         | 4 ++--
 bundle.h         | 4 ++--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 9e9c65d9c6..1ea4bfdfc1 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -56,8 +56,7 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 		}
 		if (!startup_info->have_repository)
 			die(_("Need a repository to create a bundle."));
-		return !!create_bundle(the_repository, &header,
-				       bundle_file, argc, argv);
+		return !!create_bundle(the_repository, bundle_file, argc, argv);
 	} else if (!strcmp(cmd, "unbundle")) {
 		if (!startup_info->have_repository)
 			die(_("Need a repository to unbundle."));
diff --git a/bundle.c b/bundle.c
index 37b1daa691..b45666c49b 100644
--- a/bundle.c
+++ b/bundle.c
@@ -424,8 +424,8 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 	return ref_count;
 }
 
-int create_bundle(struct repository *r, struct bundle_header *header,
-		  const char *path, int argc, const char **argv)
+int create_bundle(struct repository *r, const char *path,
+		  int argc, const char **argv)
 {
 	struct lock_file lock = LOCK_INIT;
 	int bundle_fd = -1;
diff --git a/bundle.h b/bundle.h
index 781e6f5c3a..37c37d7f65 100644
--- a/bundle.h
+++ b/bundle.h
@@ -18,8 +18,8 @@ struct bundle_header {
 
 int is_bundle(const char *path, int quiet);
 int read_bundle_header(const char *path, struct bundle_header *header);
-int create_bundle(struct repository *r, struct bundle_header *header,
-		  const char *path, int argc, const char **argv);
+int create_bundle(struct repository *r, const char *path,
+		  int argc, const char **argv);
 int verify_bundle(struct repository *r, struct bundle_header *header, int verbose);
 #define BUNDLE_VERBOSE 1
 int unbundle(struct repository *r, struct bundle_header *header,
-- 
2.20.1.842.g8986705066

[PATCH 4/8] column: drop unused "opts" parameter in item_length()

[Jeff King]

There are no column options which impact the length computation. In
theory there might be, but this is a file-local function, so it will be
trivial to re-add the parameter should it ever be useful.

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

diff --git a/column.c b/column.c
index 2165297608..7a17c14b82 100644
--- a/column.c
+++ b/column.c
@@ -21,7 +21,7 @@ struct column_data {
 };
 
 /* return length of 's' in letters, ANSI escapes stripped */
-static int item_length(unsigned int colopts, const char *s)
+static int item_length(const char *s)
 {
 	int len, i = 0;
 	struct strbuf str = STRBUF_INIT;
@@ -167,7 +167,7 @@ static void display_table(const struct string_list *list,
 
 	ALLOC_ARRAY(data.len, list->nr);
 	for (i = 0; i < list->nr; i++)
-		data.len[i] = item_length(colopts, list->items[i].string);
+		data.len[i] = item_length(list->items[i].string);
 
 	layout(&data, &initial_width);
 
-- 
2.20.1.842.g8986705066

[PATCH 5/8] show_date_relative(): drop unused "tz" parameter

[Jeff King]

The timestamp we receive is in epoch time, so there's no need for a
timezone parameter to interpret it. The matching show_date() uses "tz"
to show dates in author local time, but relative dates show only the
absolute time difference. The author's location is irrelevant, barring
relativistic effects from using Git close to the speed of light.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h              | 2 +-
 date.c               | 8 ++++----
 t/helper/test-date.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 49713cc5a5..8d97939c0d 100644
--- a/cache.h
+++ b/cache.h
@@ -1464,7 +1464,7 @@ struct date_mode {
 struct date_mode *date_mode_from_type(enum date_mode_type type);
 
 const char *show_date(timestamp_t time, int timezone, const struct date_mode *mode);
-void show_date_relative(timestamp_t time, int tz, const struct timeval *now,
+void show_date_relative(timestamp_t time, const struct timeval *now,
 			struct strbuf *timebuf);
 int parse_date(const char *date, struct strbuf *out);
 int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset);
diff --git a/date.c b/date.c
index 9bc15df6f9..61449f8b2e 100644
--- a/date.c
+++ b/date.c
@@ -107,9 +107,9 @@ static int local_tzoffset(timestamp_t time)
 	return offset * eastwest;
 }
 
-void show_date_relative(timestamp_t time, int tz,
-			       const struct timeval *now,
-			       struct strbuf *timebuf)
+void show_date_relative(timestamp_t time,
+			const struct timeval *now,
+			struct strbuf *timebuf)
 {
 	timestamp_t diff;
 	if (now->tv_sec < time) {
@@ -216,7 +216,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 
 		strbuf_reset(&timebuf);
 		gettimeofday(&now, NULL);
-		show_date_relative(time, tz, &now, &timebuf);
+		show_date_relative(time, &now, &timebuf);
 		return timebuf.buf;
 	}
 
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index a0837371ab..aac4d542c2 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -16,7 +16,7 @@ static void show_relative_dates(const char **argv, struct timeval *now)
 
 	for (; *argv; argv++) {
 		time_t t = atoi(*argv);
-		show_date_relative(t, 0, now, &buf);
+		show_date_relative(t, now, &buf);
 		printf("%s -> %s\n", *argv, buf.buf);
 	}
 	strbuf_release(&buf);
-- 
2.20.1.842.g8986705066

Re: [PATCH 5/8] show_date_relative(): drop unused "tz" parameter

[Derrick Stolee]

On 1/24/2019 8:12 AM, Jeff King wrote:
> The timestamp we receive is in epoch time, so there's no need for a
> timezone parameter to interpret it. The matching show_date() uses "tz"
> to show dates in author local time, but relative dates show only the
> absolute time difference. The author's location is irrelevant, barring
> relativistic effects from using Git close to the speed of light.
I fully support more humor in our commit messages.

-Stolee

[PATCH 6/8] config: drop unused parameter from maybe_remove_section()

[Jeff King]

We don't need the contents buffer to drop a section; the parse
information in the config_store_data parameter is enough for our logic.

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

diff --git a/config.c b/config.c
index ff521eb27a..24ad1a9854 100644
--- a/config.c
+++ b/config.c
@@ -2565,7 +2565,6 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
  * entry (which all are to be removed).
  */
 static void maybe_remove_section(struct config_store_data *store,
-				 const char *contents,
 				 size_t *begin_offset, size_t *end_offset,
 				 int *seen_ptr)
 {
@@ -2850,7 +2849,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 				replace_end = store.parsed[j].end;
 				copy_end = store.parsed[j].begin;
 				if (!value)
-					maybe_remove_section(&store, contents,
+					maybe_remove_section(&store,
 							     &copy_end,
 							     &replace_end, &i);
 				/*
-- 
2.20.1.842.g8986705066

[PATCH 7/8] convert: drop len parameter from conversion checks

[Jeff King]

We've already extracted the useful information into our text_stat
struct, so the length is no longer needed.

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

diff --git a/convert.c b/convert.c
index 0d89ae7c23..dafd6db643 100644
--- a/convert.c
+++ b/convert.c
@@ -92,7 +92,7 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *
  * The same heuristics as diff.c::mmfile_is_binary()
  * We treat files with bare CR as binary
  */
-static int convert_is_binary(unsigned long size, const struct text_stat *stats)
+static int convert_is_binary(const struct text_stat *stats)
 {
 	if (stats->lonecr)
 		return 1;
@@ -110,7 +110,7 @@ static unsigned int gather_convert_stats(const char *data, unsigned long size)
 	if (!data || !size)
 		return 0;
 	gather_stats(data, size, &stats);
-	if (convert_is_binary(size, &stats))
+	if (convert_is_binary(&stats))
 		ret |= CONVERT_STAT_BITS_BIN;
 	if (stats.crlf)
 		ret |= CONVERT_STAT_BITS_TXT_CRLF;
@@ -245,7 +245,7 @@ static int has_crlf_in_index(const struct index_state *istate, const char *path)
 	return has_crlf;
 }
 
-static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
+static int will_convert_lf_to_crlf(struct text_stat *stats,
 				   enum crlf_action crlf_action)
 {
 	if (output_eol(crlf_action) != EOL_CRLF)
@@ -260,7 +260,7 @@ static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
 		if (stats->lonecr || stats->crlf)
 			return 0;
 
-		if (convert_is_binary(len, stats))
+		if (convert_is_binary(stats))
 			return 0;
 	}
 	return 1;
@@ -527,7 +527,7 @@ static int crlf_to_git(const struct index_state *istate,
 	convert_crlf_into_lf = !!stats.crlf;
 
 	if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {
-		if (convert_is_binary(len, &stats))
+		if (convert_is_binary(&stats))
 			return 0;
 		/*
 		 * If the file in the index has any CR in it, do not
@@ -549,7 +549,7 @@ static int crlf_to_git(const struct index_state *istate,
 			new_stats.crlf = 0;
 		}
 		/* simulate "git checkout" */
-		if (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {
+		if (will_convert_lf_to_crlf(&new_stats, crlf_action)) {
 			new_stats.crlf += new_stats.lonelf;
 			new_stats.lonelf = 0;
 		}
@@ -601,7 +601,7 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,
 		return 0;
 
 	gather_stats(src, len, &stats);
-	if (!will_convert_lf_to_crlf(len, &stats, crlf_action))
+	if (!will_convert_lf_to_crlf(&stats, crlf_action))
 		return 0;
 
 	/* are we "faking" in place editing ? */
-- 
2.20.1.842.g8986705066

[PATCH 8/8] convert: drop path parameter from actual conversion functions

[Jeff King]

The caller is responsible for looking up the attributes,
after which point we no longer care about the path at which
the content is found.

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

diff --git a/convert.c b/convert.c
index dafd6db643..d8d7df3353 100644
--- a/convert.c
+++ b/convert.c
@@ -591,7 +591,7 @@ static int crlf_to_git(const struct index_state *istate,
 	return 1;
 }
 
-static int crlf_to_worktree(const char *path, const char *src, size_t len,
+static int crlf_to_worktree(const char *src, size_t len,
 			    struct strbuf *buf, enum crlf_action crlf_action)
 {
 	char *to_free = NULL;
@@ -1091,7 +1091,7 @@ static int count_ident(const char *cp, unsigned long size)
 	return cnt;
 }
 
-static int ident_to_git(const char *path, const char *src, size_t len,
+static int ident_to_git(const char *src, size_t len,
 			struct strbuf *buf, int ident)
 {
 	char *dst, *dollar;
@@ -1135,7 +1135,7 @@ static int ident_to_git(const char *path, const char *src, size_t len,
 	return 1;
 }
 
-static int ident_to_worktree(const char *path, const char *src, size_t len,
+static int ident_to_worktree(const char *src, size_t len,
 			     struct strbuf *buf, int ident)
 {
 	struct object_id oid;
@@ -1416,7 +1416,7 @@ int convert_to_git(const struct index_state *istate,
 			len = dst->len;
 		}
 	}
-	return ret | ident_to_git(path, src, len, dst, ca.ident);
+	return ret | ident_to_git(src, len, dst, ca.ident);
 }
 
 void convert_to_git_filter_fd(const struct index_state *istate,
@@ -1434,7 +1434,7 @@ void convert_to_git_filter_fd(const struct index_state *istate,
 
 	encode_to_git(path, dst->buf, dst->len, dst, ca.working_tree_encoding, conv_flags);
 	crlf_to_git(istate, path, dst->buf, dst->len, dst, ca.crlf_action, conv_flags);
-	ident_to_git(path, dst->buf, dst->len, dst, ca.ident);
+	ident_to_git(dst->buf, dst->len, dst, ca.ident);
 }
 
 static int convert_to_working_tree_internal(const struct index_state *istate,
@@ -1447,7 +1447,7 @@ static int convert_to_working_tree_internal(const struct index_state *istate,
 
 	convert_attrs(istate, &ca, path);
 
-	ret |= ident_to_worktree(path, src, len, dst, ca.ident);
+	ret |= ident_to_worktree(src, len, dst, ca.ident);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;
@@ -1458,7 +1458,7 @@ static int convert_to_working_tree_internal(const struct index_state *istate,
 	 * support smudge).  The filters might expect CRLFs.
 	 */
 	if ((ca.drv && (ca.drv->smudge || ca.drv->process)) || !normalizing) {
-		ret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);
+		ret |= crlf_to_worktree(src, len, dst, ca.crlf_action);
 		if (ret) {
 			src = dst->buf;
 			len = dst->len;
-- 
2.20.1.842.g8986705066

Re: [PATCH 0/9] drop some unused parameters

[Derrick Stolee]

On 1/24/2019 8:11 AM, Jeff King wrote:
>    2. Patches that drop unused parameters (i.e., code cleanup).
I've read the patches and they do exactly this, along with explanations 
of why they don't fit in the first category.

Thanks,
-Stolee

Re: [PATCH 0/9] drop some unused parameters

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The patches themselves are pretty much independent from each other. I
> based these on master. They're cleanup which _could_ go to maint, and I
> suspect they'd apply there, too (most of these are pretty old), but
> again, I don't think there's a particular urgency to this.

Thanks for being considerate.  One potential benefit that we might
gain by applying these clean-ups also to 'maint' is that it would
allow us to keep 'master' and 'maint' more in sync, so it would make
it more likely for unrelated bugfixes we may have in the future in
the vicinity to also apply cleanly to both.  By definition, the
variables removed are not used, so it would make an interesting
situation if such a future bugfix needs to look at them (sort of
turning the changes in this series into category 1 "we should have
been looking at them, but we didn't"), but at that point, we'd be
resurrecting these variables we drop here, so it won't be really
hurting us.  And if such a bugfix does not need to look at them,
then textual context that might be caused by not applying these to
'maint' but keeping them only to 'master' would not be a big deal,
either.

So I can go either way, but I am tempted to leave them outside
'maint' --- at least that is my current thinking.

Thanks.

>
>   [1/8]: match-trees: drop unused path parameter from score functions
>   [2/8]: apply: drop unused "def" parameter from find_name_gnu()
>   [3/8]: create_bundle(): drop unused "header" parameter
>   [4/8]: column: drop unused "opts" parameter in item_length()
>   [5/8]: show_date_relative(): drop unused "tz" parameter
>   [6/8]: config: drop unused parameter from maybe_remove_section()
>   [7/8]: convert: drop len parameter from conversion checks
>   [8/8]: convert: drop path parameter from actual conversion functions
>
>  apply.c              |  5 ++---
>  builtin/bundle.c     |  3 +--
>  bundle.c             |  4 ++--
>  bundle.h             |  4 ++--
>  cache.h              |  2 +-
>  column.c             |  4 ++--
>  config.c             |  3 +--
>  convert.c            | 28 ++++++++++++++--------------
>  date.c               |  8 ++++----
>  match-trees.c        | 16 +++++++---------
>  t/helper/test-date.c |  2 +-
>  11 files changed, 37 insertions(+), 42 deletions(-)

Re: [PATCH 0/9] drop some unused parameters

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]

On Thu, Jan 24, 2019 at 08:11:05AM -0500, Jeff King wrote:
> I've mentioned before that I have a series which compiles cleanly with
> -Wunused-parameters. I split this work roughly into three groups:
> 
>   1. Patches that fix bugs (i.e., we should have been using the
>      parameter but didn't).
> 
>   2. Patches that drop unused parameters (i.e., code cleanup).
> 
>   3. Patches that annotate undroppable parameters (e.g., ones that are
>      present due to a callback interface).
> 
> All of the patches from group 1 have been merged already. So this series
> starts us off on group 2. There are about 50 patches in that group.
> Given that none of them are urgent, I plan to feed them in batches to
> avoid overwhelming reviewers. I'm also ordering them to avoid conflicts
> with other topics in flight (this batch has no conflicts with 'next',
> and only one minor textual conflict with 'pu').

All of these patches seemed straightforward. I did see a few where there
may have originally been some consistency benefit to keeping parameters
(always passing file name to *_to_worktree, for example), but I'm fine
dropping them if it means we can get better development help from the
compiler.

I am also, for the record, in favor of ignoring the effects of
relativity for the purposes of Git. :)
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]