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

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

[Jeff King]

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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.

-- >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(-)

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

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

[Junio C Hamano]

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.

Thanks.  All four patches look good.

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

[Derrick Stolee]

On 10/6/22 1:02 PM, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

>> 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?

> 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

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

[Jeff King]

On Thu, Oct 06, 2022 at 10:02:33AM -0700, Junio C Hamano wrote:

> 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

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

[Jeff King]

On Thu, Oct 06, 2022 at 02:33:08PM -0400, Derrick Stolee wrote:

> 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