[PATCH 0/13] more unused parameter cleanups

[PATCH 0/13] more unused parameter cleanups

[Jeff King]

Here's another round of -Wunused-parameter cleanups. Previous rounds
were at [1] and [2]. As before, these are mostly just removals, so it's
easy to see there's no behavior change (there are a couple of cleanups
that ride along, though, so watch for those).

There are two minor conflicts when merging with pu:

  - jt/fetch-cdn-offload tweaked the "pack_lockfile" parameter to
    fetch_pack(). A few other parameters are dropped in this series.
    The textual resolution is pretty straightforward.

  - ps/stash-in-c (et al) added a new call to report_path_error() in
    builtin/stash.c, which here loses its redundant "prefix" parameter.
    There's no textual conflict, but the new call needs to drop its
    final NULL parameter in order to compile.

[1] https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net/
[2] https://public-inbox.org/git/20190124131104.GA24017@sigill.intra.peff.net/

The patches are:

  [01/13]: revision: drop some unused "revs" parameters
  [02/13]: log: drop unused rev_info from early output
  [03/13]: log: drop unused "len" from show_tagger()
  [04/13]: update-index: drop unused prefix_length parameter from do_reupdate()
  [05/13]: test-date: drop unused "now" parameter from parse_dates()
  [06/13]: unpack-trees: drop name_entry from traverse_by_cache_tree()
  [07/13]: unpack-trees: drop unused error_type parameters
  [08/13]: report_path_error(): drop unused prefix parameter
  [09/13]: fetch_pack(): drop unused parameters
  [10/13]: parse-options: drop unused ctx parameter from show_gitcomp()
  [11/13]: pretty: drop unused "type" parameter in needs_rfc2047_encoding()
  [12/13]: pretty: drop unused strbuf from parse_padding_placeholder()
  [13/13]: parse_opt_ref_sorting: always use with NONEG flag

 builtin/branch.c            |  3 +--
 builtin/checkout.c          |  2 +-
 builtin/commit.c            |  6 +++---
 builtin/fetch-pack.c        |  2 +-
 builtin/for-each-ref.c      |  3 +--
 builtin/log.c               | 18 +++++++++---------
 builtin/ls-files.c          |  2 +-
 builtin/ls-remote.c         |  3 +--
 builtin/submodule--helper.c |  2 +-
 builtin/tag.c               |  3 +--
 builtin/update-index.c      |  5 ++---
 dir.c                       |  3 +--
 dir.h                       |  2 +-
 fetch-pack.c                |  3 +--
 fetch-pack.h                |  3 +--
 parse-options.c             |  5 ++---
 pretty.c                    | 12 +++++-------
 ref-filter.c                |  9 +++++++--
 ref-filter.h                |  5 +++++
 revision.c                  | 12 ++++++------
 t/helper/test-date.c        |  4 ++--
 transport.c                 | 10 ++++------
 unpack-trees.c              |  9 +++------
 23 files changed, 60 insertions(+), 66 deletions(-)

-Peff

[PATCH 01/13] revision: drop some unused "revs" parameters

[Jeff King]

There are several internal helpers that take a rev_info struct but don't
actually look at it. While one could argue that all helpers in
revision.c should take a rev_info struct for consistency, dropping the
unused parameter makes it clear that they don't actually depend on any
other rev options.

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

diff --git a/revision.c b/revision.c
index eb8e51bc63..695021d6fc 100644
--- a/revision.c
+++ b/revision.c
@@ -1894,7 +1894,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 	return 0;
 }
 
-static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,
+static void read_pathspec_from_stdin(struct strbuf *sb,
 				     struct argv_array *prune)
 {
 	while (strbuf_getline(sb, stdin) != EOF)
@@ -1928,7 +1928,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			die("bad revision '%s'", sb.buf);
 	}
 	if (seen_dashdash)
-		read_pathspec_from_stdin(revs, &sb, prune);
+		read_pathspec_from_stdin(&sb, prune);
 
 	strbuf_release(&sb);
 	warn_on_object_refname_ambiguity = save_warning;
@@ -2748,7 +2748,7 @@ static struct merge_simplify_state *locate_simplify_state(struct rev_info *revs,
 	return st;
 }
 
-static int mark_redundant_parents(struct rev_info *revs, struct commit *commit)
+static int mark_redundant_parents(struct commit *commit)
 {
 	struct commit_list *h = reduce_heads(commit->parents);
 	int i = 0, marked = 0;
@@ -2784,7 +2784,7 @@ static int mark_redundant_parents(struct rev_info *revs, struct commit *commit)
 	return marked;
 }
 
-static int mark_treesame_root_parents(struct rev_info *revs, struct commit *commit)
+static int mark_treesame_root_parents(struct commit *commit)
 {
 	struct commit_list *p;
 	int marked = 0;
@@ -2976,8 +2976,8 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c
 	 * Detect and simplify both cases.
 	 */
 	if (1 < cnt) {
-		int marked = mark_redundant_parents(revs, commit);
-		marked += mark_treesame_root_parents(revs, commit);
+		int marked = mark_redundant_parents(commit);
+		marked += mark_treesame_root_parents(commit);
 		if (marked)
 			marked -= leave_one_treesame_to_parent(revs, commit);
 		if (marked)
-- 
2.21.0.701.g4401309e11

[PATCH 02/13] log: drop unused rev_info from early output

[Jeff King]

The early output code passes around a rev_info struct but doesn't need
it. The setup step only turns on global signal handlers, and the
"estimate" step is done completely from the rev->commits list that is
passed in separately.

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

diff --git a/builtin/log.c b/builtin/log.c
index ab859f5904..6595471ddf 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -251,7 +251,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
  * This gives a rough estimate for how many commits we
  * will print out in the list.
  */
-static int estimate_commit_count(struct rev_info *rev, struct commit_list *list)
+static int estimate_commit_count(struct commit_list *list)
 {
 	int n = 0;
 
@@ -289,7 +289,7 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
 		switch (simplify_commit(revs, commit)) {
 		case commit_show:
 			if (show_header) {
-				int n = estimate_commit_count(revs, list);
+				int n = estimate_commit_count(list);
 				show_early_header(revs, "incomplete", n);
 				show_header = 0;
 			}
@@ -333,7 +333,7 @@ static void early_output(int signal)
 	show_early_output = log_show_early;
 }
 
-static void setup_early_output(struct rev_info *rev)
+static void setup_early_output(void)
 {
 	struct sigaction sa;
 
@@ -364,7 +364,7 @@ static void setup_early_output(struct rev_info *rev)
 
 static void finish_early_output(struct rev_info *rev)
 {
-	int n = estimate_commit_count(rev, rev->commits);
+	int n = estimate_commit_count(rev->commits);
 	signal(SIGALRM, SIG_IGN);
 	show_early_header(rev, "done", n);
 }
@@ -376,7 +376,7 @@ static int cmd_log_walk(struct rev_info *rev)
 	int saved_dcctc = 0, close_file = rev->diffopt.close_file;
 
 	if (rev->early_output)
-		setup_early_output(rev);
+		setup_early_output();
 
 	if (prepare_revision_walk(rev))
 		die(_("revision walk setup failed"));
-- 
2.21.0.701.g4401309e11

[PATCH 03/13] log: drop unused "len" from show_tagger()

[Jeff King]

We pass the length of the found "tagger" line to show_tagger(), but it
does not use it; instead, it passes the string to pp_user_info(), which
reads until newline or NUL. This is OK for our purposes because we
always read the object contents into a buffer with an extra NUL (and
indeed, our sole caller already relies on this by using starts_with).

Let's drop the ignored parameter. And while we're touching the caller,
let's use skip_prefix() to avoid a magic number.

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

diff --git a/builtin/log.c b/builtin/log.c
index 6595471ddf..35314d12ec 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -490,7 +490,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 	return cmd_log_walk(&rev);
 }
 
-static void show_tagger(char *buf, int len, struct rev_info *rev)
+static void show_tagger(const char *buf, struct rev_info *rev)
 {
 	struct strbuf out = STRBUF_INIT;
 	struct pretty_print_context pp = {0};
@@ -546,11 +546,11 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
 	assert(type == OBJ_TAG);
 	while (offset < size && buf[offset] != '\n') {
 		int new_offset = offset + 1;
+		const char *ident;
 		while (new_offset < size && buf[new_offset++] != '\n')
 			; /* do nothing */
-		if (starts_with(buf + offset, "tagger "))
-			show_tagger(buf + offset + 7,
-				    new_offset - offset - 7, rev);
+		if (skip_prefix(buf + offset, "tagger ", &ident))
+			show_tagger(ident, rev);
 		offset = new_offset;
 	}
 
-- 
2.21.0.701.g4401309e11

[PATCH 04/13] update-index: drop unused prefix_length parameter from do_reupdate()

[Jeff King]

The prefix is always a NUL-terminated string, and we just end up passing
it along to parse_pathspec() anyway (which does not even take a length).

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

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1b6c42f748..ff5cfd1194 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -724,7 +724,7 @@ static int do_unresolve(int ac, const char **av,
 }
 
 static int do_reupdate(int ac, const char **av,
-		       const char *prefix, int prefix_length)
+		       const char *prefix)
 {
 	/* Read HEAD and run update-index on paths that are
 	 * merged and already different between index and HEAD.
@@ -940,8 +940,7 @@ static enum parse_opt_result reupdate_callback(
 
 	/* consume remaining arguments. */
 	setup_work_tree();
-	*has_errors = do_reupdate(ctx->argc, ctx->argv,
-				prefix, prefix ? strlen(prefix) : 0);
+	*has_errors = do_reupdate(ctx->argc, ctx->argv, prefix);
 	if (*has_errors)
 		active_cache_changed = 0;
 
-- 
2.21.0.701.g4401309e11

[PATCH 05/13] test-date: drop unused "now" parameter from parse_dates()

[Jeff King]

We only need the current time for relative dates like "5
minutes ago", and those are parsed only through approxidate,
not the strict parser used by parse_dates().

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

diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index b3253803ac..585347ea48 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -55,7 +55,7 @@ static void show_dates(const char **argv, const char *format)
 	}
 }
 
-static void parse_dates(const char **argv, struct timeval *now)
+static void parse_dates(const char **argv)
 {
 	struct strbuf result = STRBUF_INIT;
 
@@ -124,7 +124,7 @@ int cmd__date(int argc, const char **argv)
 	else if (skip_prefix(*argv, "show:", &x))
 		show_dates(argv+1, x);
 	else if (!strcmp(*argv, "parse"))
-		parse_dates(argv+1, &now);
+		parse_dates(argv+1);
 	else if (!strcmp(*argv, "approxidate"))
 		parse_approxidate(argv+1, &now);
 	else if (!strcmp(*argv, "timestamp"))
-- 
2.21.0.701.g4401309e11

[PATCH 06/13] unpack-trees: drop name_entry from traverse_by_cache_tree()

[Jeff King]

We pull the names from the existing index rather than the tree entry,
which is after all the point of this function. Let's drop the unused
"names" parameter.

Note that we leave the "nr_names" parameter, as it tells us how many
trees we are traversing (and thus how many index stages to set up).

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

diff --git a/unpack-trees.c b/unpack-trees.c
index 22c41a3ba8..79551ab9cc 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -707,7 +707,6 @@ static int index_pos_by_traverse_info(struct name_entry *names,
  * instead of ODB since we already know what these trees contain.
  */
 static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names,
-				  struct name_entry *names,
 				  struct traverse_info *info)
 {
 	struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
@@ -797,7 +796,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
 		 * unprocessed entries before 'pos'.
 		 */
 		bottom = o->cache_bottom;
-		ret = traverse_by_cache_tree(pos, nr_entries, n, names, info);
+		ret = traverse_by_cache_tree(pos, nr_entries, n, info);
 		o->cache_bottom = bottom;
 		return ret;
 	}
-- 
2.21.0.701.g4401309e11

[PATCH 07/13] unpack-trees: drop unused error_type parameters

[Jeff King]

The verify_clean_subdirectory() helper takes an error_type parameter
from the caller, but doesn't actually use it. Instead, when it calls
add_rejected_path() it passes NOT_UPTODATE_DIR, its own custom error
type which is more specific than what the caller provides. Likewise for
verify_clean_submodule(), which always passes WOULD_LOSE_SUBMODULE.

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

diff --git a/unpack-trees.c b/unpack-trees.c
index 79551ab9cc..5f65ff5804 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1758,7 +1758,6 @@ static void invalidate_ce_path(const struct cache_entry *ce,
  */
 static int verify_clean_submodule(const char *old_sha1,
 				  const struct cache_entry *ce,
-				  enum unpack_trees_error_types error_type,
 				  struct unpack_trees_options *o)
 {
 	if (!submodule_from_ce(ce))
@@ -1769,7 +1768,6 @@ static int verify_clean_submodule(const char *old_sha1,
 }
 
 static int verify_clean_subdirectory(const struct cache_entry *ce,
-				     enum unpack_trees_error_types error_type,
 				     struct unpack_trees_options *o)
 {
 	/*
@@ -1792,7 +1790,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 		if (!sub_head && oideq(&oid, &ce->oid))
 			return 0;
 		return verify_clean_submodule(sub_head ? NULL : oid_to_hex(&oid),
-					      ce, error_type, o);
+					      ce, o);
 	}
 
 	/*
@@ -1888,7 +1886,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
 		 * files that are in "foo/" we would lose
 		 * them.
 		 */
-		if (verify_clean_subdirectory(ce, error_type, o) < 0)
+		if (verify_clean_subdirectory(ce, o) < 0)
 			return -1;
 		return 0;
 	}
-- 
2.21.0.701.g4401309e11

[PATCH 08/13] report_path_error(): drop unused prefix parameter

[Jeff King]

This hasn't been used since 17ddc66e70 (convert report_path_error to
take struct pathspec, 2013-07-14), as the names in the struct will have
already been prefixed when they were parsed.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout.c          | 2 +-
 builtin/commit.c            | 6 +++---
 builtin/ls-files.c          | 2 +-
 builtin/submodule--helper.c | 2 +-
 dir.c                       | 3 +--
 dir.h                       | 2 +-
 6 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0e6037b296..72f7110fd8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -376,7 +376,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 							ps_matched,
 							opts);
 
-	if (report_path_error(ps_matched, &opts->pathspec, opts->prefix)) {
+	if (report_path_error(ps_matched, &opts->pathspec)) {
 		free(ps_matched);
 		return 1;
 	}
diff --git a/builtin/commit.c b/builtin/commit.c
index f17537474a..a138ff85b0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -235,7 +235,7 @@ static int commit_index_files(void)
  * and return the paths that match the given pattern in list.
  */
 static int list_paths(struct string_list *list, const char *with_tree,
-		      const char *prefix, const struct pathspec *pattern)
+		      const struct pathspec *pattern)
 {
 	int i, ret;
 	char *m;
@@ -264,7 +264,7 @@ static int list_paths(struct string_list *list, const char *with_tree,
 			item->util = item; /* better a valid pointer than a fake one */
 	}
 
-	ret = report_path_error(m, pattern, prefix);
+	ret = report_path_error(m, pattern);
 	free(m);
 	return ret;
 }
@@ -454,7 +454,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			die(_("cannot do a partial commit during a cherry-pick."));
 	}
 
-	if (list_paths(&partial, !current_head ? NULL : "HEAD", prefix, &pathspec))
+	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
 		exit(1);
 
 	discard_cache();
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 29a8762d46..37a08c714b 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -680,7 +680,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, &pathspec, prefix);
+		bad = report_path_error(ps_matched, &pathspec);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6bcc4f1bd7..8342b78add 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -348,7 +348,7 @@ static int module_list_compute(int argc, const char **argv,
 			i++;
 	}
 
-	if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
+	if (ps_matched && report_path_error(ps_matched, pathspec))
 		result = -1;
 
 	free(ps_matched);
diff --git a/dir.c b/dir.c
index b2cabadf25..5286f710cc 100644
--- a/dir.c
+++ b/dir.c
@@ -502,8 +502,7 @@ int submodule_path_match(const struct index_state *istate,
 }
 
 int report_path_error(const char *ps_matched,
-		      const struct pathspec *pathspec,
-		      const char *prefix)
+		      const struct pathspec *pathspec)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
diff --git a/dir.h b/dir.h
index e3ec26143d..823bae628b 100644
--- a/dir.h
+++ b/dir.h
@@ -220,7 +220,7 @@ extern int match_pathspec(const struct index_state *istate,
 			  const struct pathspec *pathspec,
 			  const char *name, int namelen,
 			  int prefix, char *seen, int is_dir);
-extern int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix);
+extern int report_path_error(const char *ps_matched, const struct pathspec *pathspec);
 extern int within_depth(const char *name, int namelen, int depth, int max_depth);
 
 extern int fill_directory(struct dir_struct *dir,
-- 
2.21.0.701.g4401309e11

[PATCH 09/13] fetch_pack(): drop unused parameters

[Jeff King]

We don't need the caller of fetch_pack() to pass in "dest", which is the
remote URL. Since ba227857d2 (Reduce the number of connects when
fetching, 2008-02-04), the caller is responsible for calling
git_connect() itself, and our "dest" parameter is unused.

That commit also started passing us the resulting "conn" child_process
from git_connect(). But likewise, we do not need do anything with it.
The descriptors in "fd" are enough for us, and the caller is responsible
for cleaning up "conn".

We can just drop both parameters.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c |  2 +-
 fetch-pack.c         |  3 +--
 fetch-pack.h         |  3 +--
 transport.c          | 10 ++++------
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 153a2bd282..dc1485c8aa 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -234,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		BUG("unknown protocol version");
 	}
 
-	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
+	ref = fetch_pack(&args, fd, ref, sought, nr_sought,
 			 &shallow, pack_lockfile_ptr, version);
 	if (pack_lockfile) {
 		printf("lock %s\n", pack_lockfile);
diff --git a/fetch-pack.c b/fetch-pack.c
index e69993b2eb..8d67d4e362 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1615,9 +1615,8 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)
 }
 
 struct ref *fetch_pack(struct fetch_pack_args *args,
-		       int fd[], struct child_process *conn,
+		       int fd[],
 		       const struct ref *ref,
-		       const char *dest,
 		       struct ref **sought, int nr_sought,
 		       struct oid_array *shallow,
 		       char **pack_lockfile,
diff --git a/fetch-pack.h b/fetch-pack.h
index 43ec344d95..67f684229a 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -78,9 +78,8 @@ struct fetch_pack_args {
  * marked as such.
  */
 struct ref *fetch_pack(struct fetch_pack_args *args,
-		       int fd[], struct child_process *conn,
+		       int fd[],
 		       const struct ref *ref,
-		       const char *dest,
 		       struct ref **sought,
 		       int nr_sought,
 		       struct oid_array *shallow,
diff --git a/transport.c b/transport.c
index d0608df5c9..365ea574c7 100644
--- a/transport.c
+++ b/transport.c
@@ -314,7 +314,6 @@ static int fetch_refs_via_pack(struct transport *transport,
 	int ret = 0;
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
-	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
 	struct ref *refs_tmp = NULL;
 
@@ -356,16 +355,16 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	switch (data->version) {
 	case protocol_v2:
-		refs = fetch_pack(&args, data->fd, data->conn,
+		refs = fetch_pack(&args, data->fd,
 				  refs_tmp ? refs_tmp : transport->remote_refs,
-				  dest, to_fetch, nr_heads, &data->shallow,
+				  to_fetch, nr_heads, &data->shallow,
 				  &transport->pack_lockfile, data->version);
 		break;
 	case protocol_v1:
 	case protocol_v0:
-		refs = fetch_pack(&args, data->fd, data->conn,
+		refs = fetch_pack(&args, data->fd,
 				  refs_tmp ? refs_tmp : transport->remote_refs,
-				  dest, to_fetch, nr_heads, &data->shallow,
+				  to_fetch, nr_heads, &data->shallow,
 				  &transport->pack_lockfile, data->version);
 		break;
 	case protocol_unknown_version:
@@ -389,7 +388,6 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	free_refs(refs_tmp);
 	free_refs(refs);
-	free(dest);
 	return ret;
 }
 
-- 
2.21.0.701.g4401309e11

[PATCH 10/13] parse-options: drop unused ctx parameter from show_gitcomp()

[Jeff King]

The completion display doesn't actually care about where we are in the
parsing. It's generated completely from the set of available options. So
we don't need to see the parse-options context struct at all.

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

diff --git a/parse-options.c b/parse-options.c
index cec74522e5..ade83a7b20 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -523,8 +523,7 @@ static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
 	}
 }
 
-static int show_gitcomp(struct parse_opt_ctx_t *ctx,
-			const struct option *opts)
+static int show_gitcomp(const struct option *opts)
 {
 	const struct option *original_opts = opts;
 	int nr_noopts = 0;
@@ -603,7 +602,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 		/* lone --git-completion-helper is asked by git-completion.bash */
 		if (ctx->total == 1 && !strcmp(arg + 1, "-git-completion-helper"))
-			return show_gitcomp(ctx, options);
+			return show_gitcomp(options);
 
 		if (arg[1] != '-') {
 			ctx->opt = arg + 1;
-- 
2.21.0.701.g4401309e11

[PATCH 11/13] pretty: drop unused "type" parameter in needs_rfc2047_encoding()

[Jeff King]

The "should we encode" check was split off from add_rfc2047() into its
own function in 41dd00bad3 (format-patch: fix rfc2047 address encoding
with respect to rfc822 specials, 2012-10-18). But only the "add" half
needs to know the rfc2047_type, since it only affects _how_ we encode,
not whether we do.

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

diff --git a/pretty.c b/pretty.c
index f496f0f128..f925a014f9 100644
--- a/pretty.c
+++ b/pretty.c
@@ -343,8 +343,7 @@ static int is_rfc2047_special(char ch, enum rfc2047_type type)
 	return !(isalnum(ch) || ch == '!' || ch == '*' || ch == '+' || ch == '-' || ch == '/');
 }
 
-static int needs_rfc2047_encoding(const char *line, int len,
-				  enum rfc2047_type type)
+static int needs_rfc2047_encoding(const char *line, int len)
 {
 	int i;
 
@@ -470,7 +469,7 @@ void pp_user_info(struct pretty_print_context *pp,
 		}
 
 		strbuf_addstr(sb, "From: ");
-		if (needs_rfc2047_encoding(namebuf, namelen, RFC2047_ADDRESS)) {
+		if (needs_rfc2047_encoding(namebuf, namelen)) {
 			add_rfc2047(sb, namebuf, namelen,
 				    encoding, RFC2047_ADDRESS);
 			max_length = 76; /* per rfc2047 */
@@ -1728,7 +1727,7 @@ void pp_title_line(struct pretty_print_context *pp,
 	if (pp->print_email_subject) {
 		if (pp->rev)
 			fmt_output_email_subject(sb, pp->rev);
-		if (needs_rfc2047_encoding(title.buf, title.len, RFC2047_SUBJECT))
+		if (needs_rfc2047_encoding(title.buf, title.len))
 			add_rfc2047(sb, title.buf, title.len,
 						encoding, RFC2047_SUBJECT);
 		else
-- 
2.21.0.701.g4401309e11

[PATCH 12/13] pretty: drop unused strbuf from parse_padding_placeholder()

[Jeff King]

Unlike other parts of the --pretty user-format expansion,
this function is not actually writing to the output, but
instead just storing the padding values into a context
struct. We don't need to be passed a strbuf at all.

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

diff --git a/pretty.c b/pretty.c
index f925a014f9..ced0485257 100644
--- a/pretty.c
+++ b/pretty.c
@@ -988,8 +988,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
 	return rest - placeholder;
 }
 
-static size_t parse_padding_placeholder(struct strbuf *sb,
-					const char *placeholder,
+static size_t parse_padding_placeholder(const char *placeholder,
 					struct format_commit_context *c)
 {
 	const char *ch = placeholder;
@@ -1194,7 +1193,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	case '<':
 	case '>':
-		return parse_padding_placeholder(sb, placeholder, c);
+		return parse_padding_placeholder(placeholder, c);
 	}
 
 	/* these depend on the commit */
-- 
2.21.0.701.g4401309e11

[PATCH 13/13] parse_opt_ref_sorting: always use with NONEG flag

[Jeff King]

The "--sort" parameter of for-each-ref, etc, does not handle negation,
and instead returns an error to the parse-options code. But neither
piece of code prints anything for the user, which may leave them
confused:

  $ git for-each-ref --no-sort
  $ echo $?
  129

As the comment in the callback function notes, this probably should
clear the list, which would make it consistent with other list-like
options (i.e., anything that uses OPT_STRING_LIST currently).
Unfortunately that's a bit tricky due to the way the ref-filter code
works. But in the meantime, let's at least make the error a little less
confusing:

  - switch to using PARSE_OPT_NONEG in the option definition, which will
    cause the options code to produce a useful message

  - since this was cut-and-pasted to four different spots, let's define
    a single OPT_REF_SORT() macro that we can use everywhere

  - the callback can use BUG_ON_OPT_NEG() to make sure the correct flags
    are used (incidentally, this also satisfies -Wunused-parameters,
    since we're now looking at "unset")

  - expand the comment into a NEEDSWORK to make it clear that the
    direction is right, but the details need to be worked out

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c       | 3 +--
 builtin/for-each-ref.c | 3 +--
 builtin/ls-remote.c    | 3 +--
 builtin/tag.c          | 3 +--
 ref-filter.c           | 9 +++++++--
 ref-filter.h           | 5 +++++
 6 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4c83055730..d4359b33ac 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -644,8 +644,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_MERGED(&filter, N_("print only branches that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only branches of the object"), 0, parse_opt_object_name
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e931be9ce4..465153e853 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -37,8 +37,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
 		OPT__COLOR(&format.use_color, N_("respect format colors")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			    N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		OPT_CALLBACK(0, "points-at", &filter.points_at,
 			     N_("object"), N_("print only refs which points at the given object"),
 			     parse_opt_object_name),
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1d7f1f5ce2..6ef519514b 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -67,8 +67,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
diff --git a/builtin/tag.c b/builtin/tag.c
index 02f6bd1279..ad97595fbf 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -412,8 +412,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
 		OPT_MERGED(&filter, N_("print only tags that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
diff --git a/ref-filter.c b/ref-filter.c
index 3aca105307..8d11a94cbd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2337,8 +2337,13 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
 
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 {
-	if (!arg) /* should --no-sort void the list ? */
-		return -1;
+	/*
+	 * NEEDSWORK: We should probably clear the list in this case, but we've
+	 * already munged the global used_atoms list, which would need to be
+	 * undone.
+	 */
+	BUG_ON_OPT_NEG(unset);
+
 	parse_ref_sorting(opt->value, arg);
 	return 0;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 85c8ebc3b9..5ba46a7e38 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -96,6 +96,11 @@ struct ref_format {
 #define OPT_MERGED(f, h) _OPT_MERGED_NO_MERGED("merged", f, h)
 #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED("no-merged", f, h)
 
+#define OPT_REF_SORT(var) \
+	OPT_CALLBACK_F(0, "sort", (var), \
+		       N_("key"), N_("field name to sort"), \
+		       PARSE_OPT_NONEG, parse_opt_ref_sorting)
+
 /*
  * API for filtering a set of refs. Based on the type of refs the user
  * has requested, we iterate through those refs and apply filters
-- 
2.21.0.701.g4401309e11

Re: [PATCH 13/13] parse_opt_ref_sorting: always use with NONEG flag

[Martin Ågren]

On Wed, 20 Mar 2019 at 09:17, Jeff King <peff@peff.net> wrote:
>   - since this was cut-and-pasted to four different spots, let's define
>     a single OPT_REF_SORT() macro that we can use everywhere

Indeed, all four are identical. And FWIW I failed to find a fifth caller
anywhere (I looked for "OPT_CALLBACK.*sort").

> -               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
> -                            N_("field name to sort on"), &parse_opt_ref_sorting),
> +               OPT_REF_SORT(sorting_tail),

> +#define OPT_REF_SORT(var) \
> +       OPT_CALLBACK_F(0, "sort", (var), \
> +                      N_("key"), N_("field name to sort"), \
> +                      PARSE_OPT_NONEG, parse_opt_ref_sorting)

This one is not identical though. ;-) You drop the "on". I trust you to
know which of these is (more) correct, but I was a bit surprised to see
"on" disappear without any mention. Mistake, or intended?

Other than that surprise ending, the whole series was a nice read.

Martin

Re: [PATCH 13/13] parse_opt_ref_sorting: always use with NONEG flag

[Jeff King]

On Wed, Mar 20, 2019 at 01:22:22PM +0100, Martin Ågren wrote:

> > +#define OPT_REF_SORT(var) \
> > +       OPT_CALLBACK_F(0, "sort", (var), \
> > +                      N_("key"), N_("field name to sort"), \
> > +                      PARSE_OPT_NONEG, parse_opt_ref_sorting)
> 
> This one is not identical though. ;-) You drop the "on". I trust you to
> know which of these is (more) correct, but I was a bit surprised to see
> "on" disappear without any mention. Mistake, or intended?

Mistake. I retyped when I should have cut-and-pasted.

Amusingly, I even carefully checked that the original four were all
identical, but didn't think to check against my new one. Revised patch
below.

> Other than that surprise ending, the whole series was a nice read.

I'm the M. Night Shyamalan of Git development.

Thanks for reading. :)

-- >8 --
Subject: [PATCH] parse_opt_ref_sorting: always use with NONEG flag

The "--sort" parameter of for-each-ref, etc, does not handle negation,
and instead returns an error to the parse-options code. But neither
piece of code prints anything for the user, which may leave them
confused:

  $ git for-each-ref --no-sort
  $ echo $?
  129

As the comment in the callback function notes, this probably should
clear the list, which would make it consistent with other list-like
options (i.e., anything that uses OPT_STRING_LIST currently).
Unfortunately that's a bit tricky due to the way the ref-filter code
works. But in the meantime, let's at least make the error a little less
confusing:

  - switch to using PARSE_OPT_NONEG in the option definition, which will
    cause the options code to produce a useful message

  - since this was cut-and-pasted to four different spots, let's define
    a single OPT_REF_SORT() macro that we can use everywhere

  - the callback can use BUG_ON_OPT_NEG() to make sure the correct flags
    are used (incidentally, this also satisfies -Wunused-parameters,
    since we're now looking at "unset")

  - expand the comment into a NEEDSWORK to make it clear that the
    direction is right, but the details need to be worked out

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c       | 3 +--
 builtin/for-each-ref.c | 3 +--
 builtin/ls-remote.c    | 3 +--
 builtin/tag.c          | 3 +--
 ref-filter.c           | 9 +++++++--
 ref-filter.h           | 5 +++++
 6 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4c83055730..d4359b33ac 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -644,8 +644,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_MERGED(&filter, N_("print only branches that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only branches of the object"), 0, parse_opt_object_name
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e931be9ce4..465153e853 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -37,8 +37,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
 		OPT__COLOR(&format.use_color, N_("respect format colors")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			    N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		OPT_CALLBACK(0, "points-at", &filter.points_at,
 			     N_("object"), N_("print only refs which points at the given object"),
 			     parse_opt_object_name),
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1d7f1f5ce2..6ef519514b 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -67,8 +67,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
diff --git a/builtin/tag.c b/builtin/tag.c
index 02f6bd1279..ad97595fbf 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -412,8 +412,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
 		OPT_MERGED(&filter, N_("print only tags that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
-		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		OPT_REF_SORT(sorting_tail),
 		{
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
diff --git a/ref-filter.c b/ref-filter.c
index 3aca105307..8d11a94cbd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2337,8 +2337,13 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
 
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 {
-	if (!arg) /* should --no-sort void the list ? */
-		return -1;
+	/*
+	 * NEEDSWORK: We should probably clear the list in this case, but we've
+	 * already munged the global used_atoms list, which would need to be
+	 * undone.
+	 */
+	BUG_ON_OPT_NEG(unset);
+
 	parse_ref_sorting(opt->value, arg);
 	return 0;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 85c8ebc3b9..f1dcff4c6e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -96,6 +96,11 @@ struct ref_format {
 #define OPT_MERGED(f, h) _OPT_MERGED_NO_MERGED("merged", f, h)
 #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED("no-merged", f, h)
 
+#define OPT_REF_SORT(var) \
+	OPT_CALLBACK_F(0, "sort", (var), \
+		       N_("key"), N_("field name to sort on"), \
+		       PARSE_OPT_NONEG, parse_opt_ref_sorting)
+
 /*
  * API for filtering a set of refs. Based on the type of refs the user
  * has requested, we iterate through those refs and apply filters
-- 
2.21.0.701.g4401309e11

Re: [PATCH 0/13] more unused parameter cleanups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's another round of -Wunused-parameter cleanups. Previous rounds
> were at [1] and [2]. As before, these are mostly just removals, so it's
> easy to see there's no behavior change (there are a couple of cleanups
> that ride along, though, so watch for those).
>
> There are two minor conflicts when merging with pu:
>
>   - jt/fetch-cdn-offload tweaked the "pack_lockfile" parameter to
>     fetch_pack(). A few other parameters are dropped in this series.
>     The textual resolution is pretty straightforward.
>
>   - ps/stash-in-c (et al) added a new call to report_path_error() in
>     builtin/stash.c, which here loses its redundant "prefix" parameter.
>     There's no textual conflict, but the new call needs to drop its
>     final NULL parameter in order to compile.
>
> [1] https://public-inbox.org/git/20190214054736.GA20091@sigill.intra.peff.net/
> [2] https://public-inbox.org/git/20190124131104.GA24017@sigill.intra.peff.net/
>
> The patches are:
>
>   [01/13]: revision: drop some unused "revs" parameters
>   [02/13]: log: drop unused rev_info from early output
>   [03/13]: log: drop unused "len" from show_tagger()
>   [04/13]: update-index: drop unused prefix_length parameter from do_reupdate()
>   [05/13]: test-date: drop unused "now" parameter from parse_dates()
>   [06/13]: unpack-trees: drop name_entry from traverse_by_cache_tree()
>   [07/13]: unpack-trees: drop unused error_type parameters
>   [08/13]: report_path_error(): drop unused prefix parameter
>   [09/13]: fetch_pack(): drop unused parameters
>   [10/13]: parse-options: drop unused ctx parameter from show_gitcomp()
>   [11/13]: pretty: drop unused "type" parameter in needs_rfc2047_encoding()
>   [12/13]: pretty: drop unused strbuf from parse_padding_placeholder()
>   [13/13]: parse_opt_ref_sorting: always use with NONEG flag

Nicely written.  Thanks.

>  builtin/branch.c            |  3 +--
>  builtin/checkout.c          |  2 +-
>  builtin/commit.c            |  6 +++---
>  builtin/fetch-pack.c        |  2 +-
>  builtin/for-each-ref.c      |  3 +--
>  builtin/log.c               | 18 +++++++++---------
>  builtin/ls-files.c          |  2 +-
>  builtin/ls-remote.c         |  3 +--
>  builtin/submodule--helper.c |  2 +-
>  builtin/tag.c               |  3 +--
>  builtin/update-index.c      |  5 ++---
>  dir.c                       |  3 +--
>  dir.h                       |  2 +-
>  fetch-pack.c                |  3 +--
>  fetch-pack.h                |  3 +--
>  parse-options.c             |  5 ++---
>  pretty.c                    | 12 +++++-------
>  ref-filter.c                |  9 +++++++--
>  ref-filter.h                |  5 +++++
>  revision.c                  | 12 ++++++------
>  t/helper/test-date.c        |  4 ++--
>  transport.c                 | 10 ++++------
>  unpack-trees.c              |  9 +++------
>  23 files changed, 60 insertions(+), 66 deletions(-)
>
> -Peff

Re: [PATCH 0/13] more unused parameter cleanups

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 20 2019, Jeff King wrote:

> Here's another round of -Wunused-parameter cleanups. Previous rounds
> were at [1] and [2]. As before, these are mostly just removals, so it's
> easy to see there's no behavior change (there are a couple of cleanups
> that ride along, though, so watch for those).
>
> There are two minor conflicts when merging with pu:
>
>   - jt/fetch-cdn-offload tweaked the "pack_lockfile" parameter to
>     fetch_pack(). A few other parameters are dropped in this series.
>     The textual resolution is pretty straightforward.
>
>   - ps/stash-in-c (et al) added a new call to report_path_error() in
>     builtin/stash.c, which here loses its redundant "prefix" parameter.
>     There's no textual conflict, but the new call needs to drop its
>     final NULL parameter in order to compile.

LGTM from skimming it, FWIW this is now what we need to compile cleanly
with -Wextra:

    make DEVELOPER=1 DEVOPTS="extra-all" CFLAGS="-Wno-unused-parameter -Wno-missing-field-initializers -Wno-sign-compare -Wno-empty-body" all

For some such as -Wempty-body we'd really need to contort ourselves to
get it passing anywhere near cleanly (all of those have existing "/*
this is intentional! */" comments).

I wonder if for the rest of these it's worth re-picking up this old
suggestions of yours about #pragma:
https://public-inbox.org/git/20170126143252.ne533mcv3n2ksbai@sigill.intra.peff.net/

I.e. for us to define our own macro for these cases & use it.

https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html
https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas

I was looking into this for SunCC the other day, it has various new
warnings that are useful that neither gcc nor clang (or anything else
I've tried) has, but also has some stupidities due to faulty code
analysis, luckily those can be disabled:

https://docs.oracle.com/cd/E19205-01/819-5265/bjaby/index.html

This would allow me to compile there with -Werror.

It would mean quite some macro verbosity in some existing code, maybe
it's not worth it...

Re: [PATCH 0/13] more unused parameter cleanups

[Jeff King]

On Thu, Mar 21, 2019 at 09:50:13AM +0100, Ævar Arnfjörð Bjarmason wrote:

> LGTM from skimming it, FWIW this is now what we need to compile cleanly
> with -Wextra:
> 
>     make DEVELOPER=1 DEVOPTS="extra-all" CFLAGS="-Wno-unused-parameter -Wno-missing-field-initializers -Wno-sign-compare -Wno-empty-body" all
> 
> For some such as -Wempty-body we'd really need to contort ourselves to
> get it passing anywhere near cleanly (all of those have existing "/*
> this is intentional! */" comments).

I think we could probably define a NOOP_BODY macro or function and use
that instead. But it may not be worth the trouble. I'd have to see how
painful that would be, and whether it might find any cases that actually
look like real bugs.

For -Wunused-parameter I am working towards being able to actually
enable that everywhere. It is not _too_ bad to annotate the instances
which must be there, and my digging with it has uncovered several real
bugs. Right now I'm in the "drop useless parameters" phase, which I
expect will take one or two more rounds.

Then I'll start on the "annotate unused ones we must keep" series, which
culminates in actually flipping on the switch with DEVELOPER (or rather,
stopping flipping it off).

You can see my progress on the jk/unused-4-mark branch of
https://github.com/peff/git (I think the contents are good, but the
commit messages and organization need some cleanup).

> I wonder if for the rest of these it's worth re-picking up this old
> suggestions of yours about #pragma:
> https://public-inbox.org/git/20170126143252.ne533mcv3n2ksbai@sigill.intra.peff.net/
> 
> I.e. for us to define our own macro for these cases & use it.

The push/pop ones may be of use (which both clang and gcc seem to
support), since that would let us localize the effects. I think in many
cases there's usually a more readable solution, though (e.g., you'd want
to annotate specific parameters as unused with single word, not a 3-line
push-diag/declare-param/pop-diag mess).

-Peff