threads / patch / 58572

patch, 4 partsa few small unused-parameter fixes

Subject: [PATCH 0/4] a few small unused-parameter fixes

## tl;dr

11 messages between Oct 6, 2022 and Oct 11, 2022. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Jeff King· Oct 6, 2022, 13:09 UTC · lore

These ones are not annotations of unused parameters, but rather code changes that (I hope) make the world a better place while also silencing -Wunused-parameters.

I've been trying to bubble these up to the front of my series of UNUSED annotations. I _thought_ I had got all of them, but I think a few of these are new, and some are ones that just got lost in the shuffle of earlier series.

  [1/4]: test-submodule: inline resolve_relative_url() function
  [2/4]: multi-pack-index: avoid writing to global in option callback
  [3/4]: commit: avoid writing to global in option callback
  [4/4]: attr: convert DEBUG_ATTR to use trace API
 attr.c                     | 38 +++++++++++++++++---------------------
 builtin/commit.c           |  4 ++--
 builtin/multi-pack-index.c |  7 ++++---
 t/helper/test-submodule.c  | 22 ++++++++--------------
 4 files changed, 31 insertions(+), 40 deletions(-)
-Peff
Jeff King· Oct 6, 2022, 13:10 UTC · re: Jeff King · lore

[PATCH 1/4] test-submodule: inline resolve_relative_url() function

The resolve_relative_url() function takes argc and argv parameters; it then reads up to 3 elements of argv without looking at argc at all. At first glance, this seems like a bug. But it has only one caller, cmd__submodule_resolve_relative_url(), which does confirm that argc is 3.

The main reason this is a separate function is that it was moved from library code in 96a28a9bc6 (submodule--helper: move "resolve-relative-url-test" to a test-tool, 2022-09-01).

We can make this code simpler and more obviously safe by just inlining the function in its caller. As a bonus, this silences a -Wunused-parameter warning.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-submodule.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)
Show changes to t/helper/test-submodule.c +8 −14
diff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c
index e0e0c53d38..b7d117cd55 100644
--- a/t/helper/test-submodule.c
+++ b/t/helper/test-submodule.c
@@ -85,10 +85,17 @@ static int cmd__submodule_is_active(int argc, const char **argv)
 	return !is_submodule_active(the_repository, argv[0]);
 }
 
-static int resolve_relative_url(int argc, const char **argv)
+static int cmd__submodule_resolve_relative_url(int argc, const char **argv)
 {
 	char *remoteurl, *res;
 	const char *up_path, *url;
+	struct option options[] = {
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, "test-tools", options,
+			     submodule_resolve_relative_url_usage, 0);
+	if (argc != 3)
+		usage_with_options(submodule_resolve_relative_url_usage, options);
 
 	up_path = argv[0];
 	remoteurl = xstrdup(argv[1]);
@@ -104,19 +111,6 @@ static int resolve_relative_url(int argc, const char **argv)
 	return 0;
 }
 
-static int cmd__submodule_resolve_relative_url(int argc, const char **argv)
-{
-	struct option options[] = {
-		OPT_END()
-	};
-	argc = parse_options(argc, argv, "test-tools", options,
-			     submodule_resolve_relative_url_usage, 0);
-	if (argc != 3)
-		usage_with_options(submodule_resolve_relative_url_usage, options);
-
-	return resolve_relative_url(argc, argv);
-}
-
 static struct test_cmd cmds[] = {
 	{ "check-name", cmd__submodule_check_name },
 	{ "is-active", cmd__submodule_is_active },
-- 
2.38.0.661.g581b1c9920
Jeff King· Oct 6, 2022, 13:10 UTC · re: Jeff King · lore

[PATCH 2/4] multi-pack-index: avoid writing to global in option callback

We declare the --object-dir option like:
  OPT_CALLBACK(0, "object-dir", &opts.object_dir, ...);

but the pointer to opts.object_dir is completely unused. Instead, the callback writes directly to a global. Which fortunately happens to be opts.object_dir. So everything works as expected, but it's unnecessarily confusing.

Instead, let's have the callback write to the option value pointer that has been passed in. This also quiets a -Wunused-parameter warning (since we don't otherwise look at "opt").

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/multi-pack-index.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Show changes to builtin/multi-pack-index.c +4 −3
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 9b126d6ce0..9a18a82b05 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -56,11 +56,12 @@ static struct opts_multi_pack_index {
 static int parse_object_dir(const struct option *opt, const char *arg,
 			    int unset)
 {
-	free(opts.object_dir);
+	char **value = opt->value;
+	free(*value);
 	if (unset)
-		opts.object_dir = xstrdup(get_object_directory());
+		*value = xstrdup(get_object_directory());
 	else
-		opts.object_dir = real_pathdup(arg, 1);
+		*value = real_pathdup(arg, 1);
 	return 0;
 }
 
-- 
2.38.0.661.g581b1c9920
Jeff King· Oct 6, 2022, 13:11 UTC · re: Jeff King · lore

[PATCH 3/4] commit: avoid writing to global in option callback

The callback function for --trailer writes directly to the global trailer_args and ignores opt->value completely. This is OK, since that's where we expect to find the value. But it does mean the option declaration isn't as clear. E.g., we have:

    OPT_BOOL(0, "reset-author", &renew_authorship, ...),
    OPT_CALLBACK_F(0, "trailer", NULL, ..., opt_pass_trailer)

In the first one we can see where the result will be stored, but in the second, we get only NULL, and you have to go read the callback.

Let's pass &trailer_args, and use it in the callback. As a bonus, this silences a -Wunused-parameter warning.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/commit.c +2 −2
diff --git a/builtin/commit.c b/builtin/commit.c
index fcf9c85947..d9de4ef008 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -139,7 +139,7 @@ static int opt_pass_trailer(const struct option *opt, const char *arg, int unset
 {
 	BUG_ON_OPT_NEG(unset);
 
-	strvec_pushl(&trailer_args, "--trailer", arg, NULL);
+	strvec_pushl(opt->value, "--trailer", arg, NULL);
 	return 0;
 }
 
@@ -1633,7 +1633,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "fixup", &fixup_message, N_("[(amend|reword):]commit"), N_("use autosquash formatted message to fixup or amend/reword specified commit")),
 		OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")),
 		OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")),
-		OPT_CALLBACK_F(0, "trailer", NULL, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
+		OPT_CALLBACK_F(0, "trailer", &trailer_args, N_("trailer"), N_("add custom trailer(s)"), PARSE_OPT_NONEG, opt_pass_trailer),
 		OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")),
 		OPT_FILENAME('t', "template", &template_file, N_("use specified template file")),
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
-- 
2.38.0.661.g581b1c9920
Jeff King· Oct 6, 2022, 13:13 UTC · re: Jeff King · lore

[PATCH 4/4] attr: convert DEBUG_ATTR to use trace API

If DEBUG_ATTR is defined, we print some debugging information to stderr. Compared to our usual trace infrastructure, this has a few downsides:

  - it's pretty inaccessible. You have to rebuild Git to enable it
    (compared to setting an environment variable)
  - the code isn't normally built, so it may bitrot. And indeed, it
    doesn't compile with -Werror now (one of the function parameters
    needs to be marked as const).
  - it confuses -Wunused-parameter; the "what" parameter to fill_one()
    is used only by the debug code, which is a noop macro in most builds

We can easily convert this to use the trace API, which solves all of them. The runtime impact should be minimal, as it introduces only a few extra conditionals when tracing is disabled.

Signed-off-by: Jeff King <peff@peff.net>
---
The other obvious option is to just delete this debug code, and remove
the unused parameter. I'm not sure if the trace would ever be useful or
not, and I am mostly retaining it out of the logic of "well, somebody
bothered to write it". I think the const issue has been there since
e810e06357 (attr: tighten const correctness with git_attr and
match_attr, 2017-01-27).
 attr.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)
Show changes to attr.c +17 −21
diff --git a/attr.c b/attr.c
index 8250b06953..5568294daa 100644
--- a/attr.c
+++ b/attr.c
@@ -23,10 +23,6 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 #define ATTR__UNSET NULL
 #define ATTR__UNKNOWN git_attr__unknown
 
-#ifndef DEBUG_ATTR
-#define DEBUG_ATTR 0
-#endif
-
 struct git_attr {
 	int attr_nr; /* unique attribute number */
 	char name[FLEX_ARRAY]; /* attribute name */
@@ -807,12 +803,16 @@ static struct attr_stack *read_attr(struct index_state *istate,
 	return res;
 }
 
-#if DEBUG_ATTR
-static void debug_info(const char *what, struct attr_stack *elem)
+static struct trace_key trace_attr = TRACE_KEY_INIT(ATTR);
+
+static void trace_attr_info(const char *what, struct attr_stack *elem)
 {
-	fprintf(stderr, "%s: %s\n", what, elem->origin ? elem->origin : "()");
+	trace_printf_key(&trace_attr, "%s: %s", what,
+			 elem->origin ? elem->origin : "()");
 }
-static void debug_set(const char *what, const char *match, struct git_attr *attr, const void *v)
+
+static void trace_attr_set(const char *what, const char *match,
+			   const struct git_attr *attr, const void *v)
 {
 	const char *value = v;
 
@@ -823,16 +823,12 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr
 	else if (ATTR_UNSET(value))
 		value = "unspecified";
 
-	fprintf(stderr, "%s: %s => %s (%s)\n",
-		what, attr->name, (char *) value, match);
+	trace_printf_key(&trace_attr, "%s: %s => %s (%s)\n",
+			 what, attr->name, value, match);
 }
-#define debug_push(a) debug_info("push", (a))
-#define debug_pop(a) debug_info("pop", (a))
-#else
-#define debug_push(a) do { ; } while (0)
-#define debug_pop(a) do { ; } while (0)
-#define debug_set(a,b,c,d) do { ; } while (0)
-#endif /* DEBUG_ATTR */
+
+#define trace_attr_push(a) trace_attr_info("push", (a))
+#define trace_attr_pop(a) trace_attr_info("pop", (a))
 
 static const char *git_etc_gitattributes(void)
 {
@@ -954,7 +950,7 @@ static void prepare_attr_stack(struct index_state *istate,
 		    (!namelen || path[namelen] == '/'))
 			break;
 
-		debug_pop(elem);
+		trace_attr_pop(elem);
 		*stack = elem->prev;
 		attr_stack_free(elem);
 	}
@@ -1039,9 +1035,9 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs,
 		const char *v = a->state[i].setto;
 
 		if (*n == ATTR__UNKNOWN) {
-			debug_set(what,
-				  a->is_macro ? a->u.attr->name : a->u.pat.pattern,
-				  attr, v);
+			trace_attr_set(what,
+				       a->is_macro ? a->u.attr->name : a->u.pat.pattern,
+				       attr, v);
 			*n = v;
 			rem--;
 			rem = macroexpand_one(all_attrs, attr->attr_nr, rem);
-- 
2.38.0.661.g581b1c9920
Jeff King· Oct 6, 2022, 13:23 UTC · re: Jeff King · lore

[PATCH 4alt/4] attr: drop DEBUG_ATTR code

On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:
Show 6 quoted lines
> The other obvious option is to just delete this debug code, and remove
> the unused parameter. I'm not sure if the trace would ever be useful or
> not, and I am mostly retaining it out of the logic of "well, somebody
> bothered to write it". I think the const issue has been there since
> e810e06357 (attr: tighten const correctness with git_attr and
> match_attr, 2017-01-27).
And here's what that would look like.
-- >8 --
Subject: [PATCH] attr: drop DEBUG_ATTR code

Since its inception in d0bfd026a8 (Add basic infrastructure to assign attributes to paths, 2007-04-12), the attribute code carries a little bit of debug code that is conditionally compiled only when DEBUG_ATTR is set. But since you have to know about it and make a special build of Git to use it, it's not clear that it's helping anyone (and there are very few mentions of it on the list over the years).

Meanwhile, it causes slight headaches. Since it's not built as part of a regular compile, it's subject to bitrot. E.g., this was dealt with in 712efb1a42 (attr: make it build with DEBUG_ATTR again, 2013-01-15), and it currently fails to build with DEVELOPER=1 since e810e06357 (attr: tighten const correctness with git_attr and match_attr, 2017-01-27).

And it causes confusion with -Wunused-parameter; the "what" parameter of fill_one() is unused in a normal build, but needed in a debug build.

Let's just get rid of this code (and the now-useless parameter).
Signed-off-by: Jeff King <peff@peff.net>
---
 attr.c | 41 +++--------------------------------------
 1 file changed, 3 insertions(+), 38 deletions(-)
Show changes to attr.c +3 −38
diff --git a/attr.c b/attr.c
index 8250b06953..42ad6de8c7 100644
--- a/attr.c
+++ b/attr.c
@@ -23,10 +23,6 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 #define ATTR__UNSET NULL
 #define ATTR__UNKNOWN git_attr__unknown
 
-#ifndef DEBUG_ATTR
-#define DEBUG_ATTR 0
-#endif
-
 struct git_attr {
 	int attr_nr; /* unique attribute number */
 	char name[FLEX_ARRAY]; /* attribute name */
@@ -807,33 +803,6 @@ static struct attr_stack *read_attr(struct index_state *istate,
 	return res;
 }
 
-#if DEBUG_ATTR
-static void debug_info(const char *what, struct attr_stack *elem)
-{
-	fprintf(stderr, "%s: %s\n", what, elem->origin ? elem->origin : "()");
-}
-static void debug_set(const char *what, const char *match, struct git_attr *attr, const void *v)
-{
-	const char *value = v;
-
-	if (ATTR_TRUE(value))
-		value = "set";
-	else if (ATTR_FALSE(value))
-		value = "unset";
-	else if (ATTR_UNSET(value))
-		value = "unspecified";
-
-	fprintf(stderr, "%s: %s => %s (%s)\n",
-		what, attr->name, (char *) value, match);
-}
-#define debug_push(a) debug_info("push", (a))
-#define debug_pop(a) debug_info("pop", (a))
-#else
-#define debug_push(a) do { ; } while (0)
-#define debug_pop(a) do { ; } while (0)
-#define debug_set(a,b,c,d) do { ; } while (0)
-#endif /* DEBUG_ATTR */
-
 static const char *git_etc_gitattributes(void)
 {
 	static const char *system_wide;
@@ -954,7 +923,6 @@ static void prepare_attr_stack(struct index_state *istate,
 		    (!namelen || path[namelen] == '/'))
 			break;
 
-		debug_pop(elem);
 		*stack = elem->prev;
 		attr_stack_free(elem);
 	}
@@ -1028,7 +996,7 @@ static int path_matches(const char *pathname, int pathlen,
 
 static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem);
 
-static int fill_one(const char *what, struct all_attrs_item *all_attrs,
+static int fill_one(struct all_attrs_item *all_attrs,
 		    const struct match_attr *a, int rem)
 {
 	int i;
@@ -1039,9 +1007,6 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs,
 		const char *v = a->state[i].setto;
 
 		if (*n == ATTR__UNKNOWN) {
-			debug_set(what,
-				  a->is_macro ? a->u.attr->name : a->u.pat.pattern,
-				  attr, v);
 			*n = v;
 			rem--;
 			rem = macroexpand_one(all_attrs, attr->attr_nr, rem);
@@ -1064,7 +1029,7 @@ static int fill(const char *path, int pathlen, int basename_offset,
 				continue;
 			if (path_matches(path, pathlen, basename_offset,
 					 &a->u.pat, base, stack->originlen))
-				rem = fill_one("fill", all_attrs, a, rem);
+				rem = fill_one(all_attrs, a, rem);
 		}
 	}
 
@@ -1076,7 +1041,7 @@ static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem)
 	const struct all_attrs_item *item = &all_attrs[nr];
 
 	if (item->macro && item->value == ATTR__TRUE)
-		return fill_one("expand", all_attrs, item->macro, rem);
+		return fill_one(all_attrs, item->macro, rem);
 	else
 		return rem;
 }
-- 
2.38.0.661.g581b1c9920
Junio C Hamano· Oct 6, 2022, 17:02 UTC · re: Jeff King · lore

Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:
>
>> The other obvious option is to just delete this debug code, and remove
>> the unused parameter. I'm not sure if the trace would ever be useful or
>> not, and I am mostly retaining it out of the logic of "well, somebody
>> bothered to write it". I think the const issue has been there since
>> e810e06357 (attr: tighten const correctness with git_attr and
>> match_attr, 2017-01-27).
>
> And here's what that would look like.

I highly suspect that I was the one who bothered, and while I admit it was useful while developing the attribute subsystem, I haven't needed it for the past 10 or so years.

So unless there are some folks who want to throw everything into the trace2 floodstream, I would prefer this alternative over the other one.

Thanks.  All four patches look good.
Derrick Stolee· Oct 6, 2022, 18:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code

On 10/6/22 1:02 PM, Junio C Hamano wrote:
Show 20 quoted lines
> Jeff King <peff@peff.net> writes:
> 
>> On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:
>>
>>> The other obvious option is to just delete this debug code, and remove
>>> the unused parameter. I'm not sure if the trace would ever be useful or
>>> not, and I am mostly retaining it out of the logic of "well, somebody
>>> bothered to write it". I think the const issue has been there since
>>> e810e06357 (attr: tighten const correctness with git_attr and
>>> match_attr, 2017-01-27).
>>
>> And here's what that would look like.
> 
> I highly suspect that I was the one who bothered, and while I admit
> it was useful while developing the attribute subsystem, I haven't
> needed it for the past 10 or so years.
> 
> So unless there are some folks who want to throw everything into the
> trace2 floodstream, I would prefer this alternative over the other
> one.

Are you implying that you want to use the second version, that deletes the information entirely? I'm leaning towards deleting it.

If not, and we should keep using traces, I do notice that the original version of the patch uses trace_printf_key() instead of a trace2 method. I think this is fine, too, since it's likely only to be used by Git developers, who could look for which type of trace to use.

Thanks, -Stolee

Junio C Hamano· Oct 6, 2022, 18:52 UTC · re: Derrick Stolee · lore

Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code

Derrick Stolee <derrickstolee@github.com> writes:
Show 11 quoted lines
>> I highly suspect that I was the one who bothered, and while I admit
>> it was useful while developing the attribute subsystem, I haven't
>> needed it for the past 10 or so years.
>> 
>> So unless there are some folks who want to throw everything into the
>> trace2 floodstream, I would prefer this alternative over the other
>> one.
>
> Are you implying that you want to use the second version, that
> deletes the information entirely? I'm leaning towards deleting
> it.

Sorry if I were not clear, but I would vote for using 4alt/4 and remove debugging code. Unless there are folks who want to keep it, in which case I think trace2 is fine and I won't insist on removing what those folks, if any, want to keep. Between trace and trace2, I do not have a strong opinion but if we were adding something new, we would be adding to the latter?

Show 8 quoted lines
> If not, and we should keep using traces, I do notice that the
> original version of the patch uses trace_printf_key() instead
> of a trace2 method. I think this is fine, too, since it's
> likely only to be used by Git developers, who could look for
> which type of trace to use.
>
> Thanks,
> -Stolee
Jeff King· Oct 11, 2022, 00:26 UTC · re: Derrick Stolee · lore

Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code

On Thu, Oct 06, 2022 at 02:33:08PM -0400, Derrick Stolee wrote:
Show 9 quoted lines
> Are you implying that you want to use the second version, that
> deletes the information entirely? I'm leaning towards deleting
> it.
> 
> If not, and we should keep using traces, I do notice that the
> original version of the patch uses trace_printf_key() instead
> of a trace2 method. I think this is fine, too, since it's
> likely only to be used by Git developers, who could look for
> which type of trace to use.

I think it's moot, because we're all in agreement that the code should be deleted. But...

...it is not clear to me that GIT_TRACE_* is dead for further additions. I find it delightfully simple to use, and trace2 often rather baroque and complicated, since it is aimed more at structured data, as well as storing or even transmitting data.

I'm not sure if we have an overall plan there.
-Peff
Jeff King· Oct 11, 2022, 00:23 UTC · re: Junio C Hamano · lore

Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code

On Thu, Oct 06, 2022 at 10:02:33AM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:
> >
> >> The other obvious option is to just delete this debug code, and remove
> >> the unused parameter. I'm not sure if the trace would ever be useful or
> >> not, and I am mostly retaining it out of the logic of "well, somebody
> >> bothered to write it". I think the const issue has been there since
> >> e810e06357 (attr: tighten const correctness with git_attr and
> >> match_attr, 2017-01-27).
> >
> > And here's what that would look like.
> 
> I highly suspect that I was the one who bothered, and while I admit
> it was useful while developing the attribute subsystem, I haven't
> needed it for the past 10 or so years.

It was indeed you. :) I am quite happy to delete the code (via 4alt), but didn't want to step on any toes.

-Peff

← back to recent threads