{"thread":{"id":"58572","subject":"[PATCH 0/4] a few small unused-parameter fixes","startedAt":"2022-10-06T13:10:20Z","lastAt":"2022-10-11T00:26:58Z","messageCount":11,"participants":["Jeff King","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"464286","messageId":"Yz7Tjy7Rh8cXVxYQ@coredump.intra.peff.net","threadId":"58572","inReplyTo":null,"subject":"[PATCH 0/4] a few small unused-parameter fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-06T13:09:35Z","receivedAt":"2022-10-06T13:10:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These ones are not annotations of unused parameters, but rather code\nchanges that (I hope) make the world a better place while also silencing\n-Wunused-parameters.\n\nI've been trying to bubble these up to the front of my series of UNUSED\nannotations. I _thought_ I had got all of them, but I think a few of\nthese are new, and some are ones that just got lost in the shuffle of\nearlier series.\n\n  [1/4]: test-submodule: inline resolve_relative_url() function\n  [2/4]: multi-pack-index: avoid writing to global in option callback\n  [3/4]: commit: avoid writing to global in option callback\n  [4/4]: attr: convert DEBUG_ATTR to use trace API\n\n attr.c                     | 38 +++++++++++++++++---------------------\n builtin/commit.c           |  4 ++--\n builtin/multi-pack-index.c |  7 ++++---\n t/helper/test-submodule.c  | 22 ++++++++--------------\n 4 files changed, 31 insertions(+), 40 deletions(-)\n\n-Peff\n"},{"id":"464287","messageId":"Yz7Tt4ezU/GS3tHu@coredump.intra.peff.net","threadId":"58572","inReplyTo":"Yz7Tjy7Rh8cXVxYQ@coredump.intra.peff.net","subject":"[PATCH 1/4] test-submodule: inline resolve_relative_url() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-06T13:10:15Z","receivedAt":"2022-10-06T13:10:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The resolve_relative_url() function takes argc and argv parameters; it\nthen reads up to 3 elements of argv without looking at argc at all. At\nfirst glance, this seems like a bug. But it has only one caller,\ncmd__submodule_resolve_relative_url(), which does confirm that argc is\n3.\n\nThe main reason this is a separate function is that it was moved from\nlibrary code in 96a28a9bc6 (submodule--helper: move\n\"resolve-relative-url-test\" to a test-tool, 2022-09-01).\n\nWe can make this code simpler and more obviously safe by just inlining\nthe function in its caller. As a bonus, this silences a\n-Wunused-parameter warning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-submodule.c | 22 ++++++++--------------\n 1 file changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/t/helper/test-submodule.c b/t/helper/test-submodule.c\nindex e0e0c53d38..b7d117cd55 100644\n--- a/t/helper/test-submodule.c\n+++ b/t/helper/test-submodule.c\n@@ -85,10 +85,17 @@ static int cmd__submodule_is_active(int argc, const char **argv)\n \treturn !is_submodule_active(the_repository, argv[0]);\n }\n \n-static int resolve_relative_url(int argc, const char **argv)\n+static int cmd__submodule_resolve_relative_url(int argc, const char **argv)\n {\n \tchar *remoteurl, *res;\n \tconst char *up_path, *url;\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, \"test-tools\", options,\n+\t\t\t     submodule_resolve_relative_url_usage, 0);\n+\tif (argc != 3)\n+\t\tusage_with_options(submodule_resolve_relative_url_usage, options);\n \n \tup_path = argv[0];\n \tremoteurl = xstrdup(argv[1]);\n@@ -104,19 +111,6 @@ static int resolve_relative_url(int argc, const char **argv)\n \treturn 0;\n }\n \n-static int cmd__submodule_resolve_relative_url(int argc, const char **argv)\n-{\n-\tstruct option options[] = {\n-\t\tOPT_END()\n-\t};\n-\targc = parse_options(argc, argv, \"test-tools\", options,\n-\t\t\t     submodule_resolve_relative_url_usage, 0);\n-\tif (argc != 3)\n-\t\tusage_with_options(submodule_resolve_relative_url_usage, options);\n-\n-\treturn resolve_relative_url(argc, argv);\n-}\n-\n static struct test_cmd cmds[] = {\n \t{ \"check-name\", cmd__submodule_check_name },\n \t{ \"is-active\", cmd__submodule_is_active },\n-- \n2.38.0.661.g581b1c9920\n\n"},{"id":"464288","messageId":"Yz7T3SHEnYjDDPnn@coredump.intra.peff.net","threadId":"58572","inReplyTo":"Yz7Tjy7Rh8cXVxYQ@coredump.intra.peff.net","subject":"[PATCH 2/4] multi-pack-index: avoid writing to global in option callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-06T13:10:53Z","receivedAt":"2022-10-06T13:11:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We declare the --object-dir option like:\n\n  OPT_CALLBACK(0, \"object-dir\", &opts.object_dir, ...);\n\nbut the pointer to opts.object_dir is completely unused. Instead, the\ncallback writes directly to a global. Which fortunately happens to be\nopts.object_dir. So everything works as expected, but it's unnecessarily\nconfusing.\n\nInstead, let's have the callback write to the option value pointer that\nhas been passed in. This also quiets a -Wunused-parameter warning (since\nwe don't otherwise look at \"opt\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/multi-pack-index.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 9b126d6ce0..9a18a82b05 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -56,11 +56,12 @@ static struct opts_multi_pack_index {\n static int parse_object_dir(const struct option *opt, const char *arg,\n \t\t\t    int unset)\n {\n-\tfree(opts.object_dir);\n+\tchar **value = opt->value;\n+\tfree(*value);\n \tif (unset)\n-\t\topts.object_dir = xstrdup(get_object_directory());\n+\t\t*value = xstrdup(get_object_directory());\n \telse\n-\t\topts.object_dir = real_pathdup(arg, 1);\n+\t\t*value = real_pathdup(arg, 1);\n \treturn 0;\n }\n \n-- \n2.38.0.661.g581b1c9920\n\n"},{"id":"464289","messageId":"Yz7UA2cdxVPellOe@coredump.intra.peff.net","threadId":"58572","inReplyTo":"Yz7Tjy7Rh8cXVxYQ@coredump.intra.peff.net","subject":"[PATCH 3/4] commit: avoid writing to global in option callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-06T13:11:31Z","receivedAt":"2022-10-06T13:11:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The callback function for --trailer writes directly to the global\ntrailer_args and ignores opt->value completely. This is OK, since that's\nwhere we expect to find the value. But it does mean the option\ndeclaration isn't as clear. E.g., we have:\n\n    OPT_BOOL(0, \"reset-author\", &renew_authorship, ...),\n    OPT_CALLBACK_F(0, \"trailer\", NULL, ..., opt_pass_trailer)\n\nIn the first one we can see where the result will be stored, but in the\nsecond, we get only NULL, and you have to go read the callback.\n\nLet's pass &trailer_args, and use it in the callback. As a bonus, this\nsilences a -Wunused-parameter warning.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex fcf9c85947..d9de4ef008 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -139,7 +139,7 @@ static int opt_pass_trailer(const struct option *opt, const char *arg, int unset\n {\n \tBUG_ON_OPT_NEG(unset);\n \n-\tstrvec_pushl(&trailer_args, \"--trailer\", arg, NULL);\n+\tstrvec_pushl(opt->value, \"--trailer\", arg, NULL);\n \treturn 0;\n }\n \n@@ -1633,7 +1633,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"fixup\", &fixup_message, N_(\"[(amend|reword):]commit\"), N_(\"use autosquash formatted message to fixup or amend/reword specified commit\")),\n \t\tOPT_STRING(0, \"squash\", &squash_message, N_(\"commit\"), N_(\"use autosquash formatted message to squash specified commit\")),\n \t\tOPT_BOOL(0, \"reset-author\", &renew_authorship, N_(\"the commit is authored by me now (used with -C/-c/--amend)\")),\n-\t\tOPT_CALLBACK_F(0, \"trailer\", NULL, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, opt_pass_trailer),\n+\t\tOPT_CALLBACK_F(0, \"trailer\", &trailer_args, N_(\"trailer\"), N_(\"add custom trailer(s)\"), PARSE_OPT_NONEG, opt_pass_trailer),\n \t\tOPT_BOOL('s', \"signoff\", &signoff, N_(\"add a Signed-off-by trailer\")),\n \t\tOPT_FILENAME('t', \"template\", &template_file, N_(\"use specified template file\")),\n \t\tOPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of commit\")),\n-- \n2.38.0.661.g581b1c9920\n\n"},{"id":"464290","messageId":"Yz7UhYXvNl6+1GbZ@coredump.intra.peff.net","threadId":"58572","inReplyTo":"Yz7Tjy7Rh8cXVxYQ@coredump.intra.peff.net","subject":"[PATCH 4/4] attr: convert DEBUG_ATTR to use trace API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-06T13:13:41Z","receivedAt":"2022-10-06T13:13:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If DEBUG_ATTR is defined, we print some debugging information to stderr.\nCompared to our usual trace infrastructure, this has a few downsides:\n\n  - it's pretty inaccessible. You have to rebuild Git to enable it\n    (compared to setting an environment variable)\n\n  - the code isn't normally built, so it may bitrot. And indeed, it\n    doesn't compile with -Werror now (one of the function parameters\n    needs to be marked as const).\n\n  - it confuses -Wunused-parameter; the \"what\" parameter to fill_one()\n    is used only by the debug code, which is a noop macro in most builds\n\nWe can easily convert this to use the trace API, which solves all of\nthem. The runtime impact should be minimal, as it introduces only a few\nextra conditionals when tracing is disabled.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe other obvious option is to just delete this debug code, and remove\nthe unused parameter. I'm not sure if the trace would ever be useful or\nnot, and I am mostly retaining it out of the logic of \"well, somebody\nbothered to write it\". I think the const issue has been there since\ne810e06357 (attr: tighten const correctness with git_attr and\nmatch_attr, 2017-01-27).\n\n attr.c | 38 +++++++++++++++++---------------------\n 1 file changed, 17 insertions(+), 21 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 8250b06953..5568294daa 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -23,10 +23,6 @@ static const char git_attr__unknown[] = \"(builtin)unknown\";\n #define ATTR__UNSET NULL\n #define ATTR__UNKNOWN git_attr__unknown\n \n-#ifndef DEBUG_ATTR\n-#define DEBUG_ATTR 0\n-#endif\n-\n struct git_attr {\n \tint attr_nr; /* unique attribute number */\n \tchar name[FLEX_ARRAY]; /* attribute name */\n@@ -807,12 +803,16 @@ static struct attr_stack *read_attr(struct index_state *istate,\n \treturn res;\n }\n \n-#if DEBUG_ATTR\n-static void debug_info(const char *what, struct attr_stack *elem)\n+static struct trace_key trace_attr = TRACE_KEY_INIT(ATTR);\n+\n+static void trace_attr_info(const char *what, struct attr_stack *elem)\n {\n-\tfprintf(stderr, \"%s: %s\\n\", what, elem->origin ? elem->origin : \"()\");\n+\ttrace_printf_key(&trace_attr, \"%s: %s\", what,\n+\t\t\t elem->origin ? elem->origin : \"()\");\n }\n-static void debug_set(const char *what, const char *match, struct git_attr *attr, const void *v)\n+\n+static void trace_attr_set(const char *what, const char *match,\n+\t\t\t   const struct git_attr *attr, const void *v)\n {\n \tconst char *value = v;\n \n@@ -823,16 +823,12 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr\n \telse if (ATTR_UNSET(value))\n \t\tvalue = \"unspecified\";\n \n-\tfprintf(stderr, \"%s: %s => %s (%s)\\n\",\n-\t\twhat, attr->name, (char *) value, match);\n+\ttrace_printf_key(&trace_attr, \"%s: %s => %s (%s)\\n\",\n+\t\t\t what, attr->name, value, match);\n }\n-#define debug_push(a) debug_info(\"push\", (a))\n-#define debug_pop(a) debug_info(\"pop\", (a))\n-#else\n-#define debug_push(a) do { ; } while (0)\n-#define debug_pop(a) do { ; } while (0)\n-#define debug_set(a,b,c,d) do { ; } while (0)\n-#endif /* DEBUG_ATTR */\n+\n+#define trace_attr_push(a) trace_attr_info(\"push\", (a))\n+#define trace_attr_pop(a) trace_attr_info(\"pop\", (a))\n \n static const char *git_etc_gitattributes(void)\n {\n@@ -954,7 +950,7 @@ static void prepare_attr_stack(struct index_state *istate,\n \t\t    (!namelen || path[namelen] == '/'))\n \t\t\tbreak;\n \n-\t\tdebug_pop(elem);\n+\t\ttrace_attr_pop(elem);\n \t\t*stack = elem->prev;\n \t\tattr_stack_free(elem);\n \t}\n@@ -1039,9 +1035,9 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs,\n \t\tconst char *v = a->state[i].setto;\n \n \t\tif (*n == ATTR__UNKNOWN) {\n-\t\t\tdebug_set(what,\n-\t\t\t\t  a->is_macro ? a->u.attr->name : a->u.pat.pattern,\n-\t\t\t\t  attr, v);\n+\t\t\ttrace_attr_set(what,\n+\t\t\t\t       a->is_macro ? a->u.attr->name : a->u.pat.pattern,\n+\t\t\t\t       attr, v);\n \t\t\t*n = v;\n \t\t\trem--;\n \t\t\trem = macroexpand_one(all_attrs, attr->attr_nr, rem);\n-- \n2.38.0.661.g581b1c9920\n"},{"id":"464291","messageId":"Yz7Wx4kVbrZFfm0s@coredump.intra.peff.net","threadId":"58572","inReplyTo":"Yz7UhYXvNl6+1GbZ@coredump.intra.peff.net","subject":"[PATCH 4alt/4] attr: drop DEBUG_ATTR code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-06T13:23:19Z","receivedAt":"2022-10-06T13:23:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:\n\n> The other obvious option is to just delete this debug code, and remove\n> the unused parameter. I'm not sure if the trace would ever be useful or\n> not, and I am mostly retaining it out of the logic of \"well, somebody\n> bothered to write it\". I think the const issue has been there since\n> e810e06357 (attr: tighten const correctness with git_attr and\n> match_attr, 2017-01-27).\n\nAnd here's what that would look like.\n\n-- >8 --\nSubject: [PATCH] attr: drop DEBUG_ATTR code\n\nSince its inception in d0bfd026a8 (Add basic infrastructure to assign\nattributes to paths, 2007-04-12), the attribute code carries a little\nbit of debug code that is conditionally compiled only when DEBUG_ATTR is\nset. But since you have to know about it and make a special build of Git\nto use it, it's not clear that it's helping anyone (and there are very\nfew mentions of it on the list over the years).\n\nMeanwhile, it causes slight headaches. Since it's not built as part of a\nregular compile, it's subject to bitrot. E.g., this was dealt with in\n712efb1a42 (attr: make it build with DEBUG_ATTR again, 2013-01-15), and\nit currently fails to build with DEVELOPER=1 since e810e06357 (attr:\ntighten const correctness with git_attr and match_attr, 2017-01-27).\n\nAnd it causes confusion with -Wunused-parameter; the \"what\" parameter of\nfill_one() is unused in a normal build, but needed in a debug build.\n\nLet's just get rid of this code (and the now-useless parameter).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n attr.c | 41 +++--------------------------------------\n 1 file changed, 3 insertions(+), 38 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 8250b06953..42ad6de8c7 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -23,10 +23,6 @@ static const char git_attr__unknown[] = \"(builtin)unknown\";\n #define ATTR__UNSET NULL\n #define ATTR__UNKNOWN git_attr__unknown\n \n-#ifndef DEBUG_ATTR\n-#define DEBUG_ATTR 0\n-#endif\n-\n struct git_attr {\n \tint attr_nr; /* unique attribute number */\n \tchar name[FLEX_ARRAY]; /* attribute name */\n@@ -807,33 +803,6 @@ static struct attr_stack *read_attr(struct index_state *istate,\n \treturn res;\n }\n \n-#if DEBUG_ATTR\n-static void debug_info(const char *what, struct attr_stack *elem)\n-{\n-\tfprintf(stderr, \"%s: %s\\n\", what, elem->origin ? elem->origin : \"()\");\n-}\n-static void debug_set(const char *what, const char *match, struct git_attr *attr, const void *v)\n-{\n-\tconst char *value = v;\n-\n-\tif (ATTR_TRUE(value))\n-\t\tvalue = \"set\";\n-\telse if (ATTR_FALSE(value))\n-\t\tvalue = \"unset\";\n-\telse if (ATTR_UNSET(value))\n-\t\tvalue = \"unspecified\";\n-\n-\tfprintf(stderr, \"%s: %s => %s (%s)\\n\",\n-\t\twhat, attr->name, (char *) value, match);\n-}\n-#define debug_push(a) debug_info(\"push\", (a))\n-#define debug_pop(a) debug_info(\"pop\", (a))\n-#else\n-#define debug_push(a) do { ; } while (0)\n-#define debug_pop(a) do { ; } while (0)\n-#define debug_set(a,b,c,d) do { ; } while (0)\n-#endif /* DEBUG_ATTR */\n-\n static const char *git_etc_gitattributes(void)\n {\n \tstatic const char *system_wide;\n@@ -954,7 +923,6 @@ static void prepare_attr_stack(struct index_state *istate,\n \t\t    (!namelen || path[namelen] == '/'))\n \t\t\tbreak;\n \n-\t\tdebug_pop(elem);\n \t\t*stack = elem->prev;\n \t\tattr_stack_free(elem);\n \t}\n@@ -1028,7 +996,7 @@ static int path_matches(const char *pathname, int pathlen,\n \n static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem);\n \n-static int fill_one(const char *what, struct all_attrs_item *all_attrs,\n+static int fill_one(struct all_attrs_item *all_attrs,\n \t\t    const struct match_attr *a, int rem)\n {\n \tint i;\n@@ -1039,9 +1007,6 @@ static int fill_one(const char *what, struct all_attrs_item *all_attrs,\n \t\tconst char *v = a->state[i].setto;\n \n \t\tif (*n == ATTR__UNKNOWN) {\n-\t\t\tdebug_set(what,\n-\t\t\t\t  a->is_macro ? a->u.attr->name : a->u.pat.pattern,\n-\t\t\t\t  attr, v);\n \t\t\t*n = v;\n \t\t\trem--;\n \t\t\trem = macroexpand_one(all_attrs, attr->attr_nr, rem);\n@@ -1064,7 +1029,7 @@ static int fill(const char *path, int pathlen, int basename_offset,\n \t\t\t\tcontinue;\n \t\t\tif (path_matches(path, pathlen, basename_offset,\n \t\t\t\t\t &a->u.pat, base, stack->originlen))\n-\t\t\t\trem = fill_one(\"fill\", all_attrs, a, rem);\n+\t\t\t\trem = fill_one(all_attrs, a, rem);\n \t\t}\n \t}\n \n@@ -1076,7 +1041,7 @@ static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem)\n \tconst struct all_attrs_item *item = &all_attrs[nr];\n \n \tif (item->macro && item->value == ATTR__TRUE)\n-\t\treturn fill_one(\"expand\", all_attrs, item->macro, rem);\n+\t\treturn fill_one(all_attrs, item->macro, rem);\n \telse\n \t\treturn rem;\n }\n-- \n2.38.0.661.g581b1c9920\n\n"},{"id":"464309","messageId":"xmqqwn9clwba.fsf@gitster.g","threadId":"58572","inReplyTo":"Yz7Wx4kVbrZFfm0s@coredump.intra.peff.net","subject":"Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-06T17:02:33Z","receivedAt":"2022-10-06T17:02:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:\n>\n>> The other obvious option is to just delete this debug code, and remove\n>> the unused parameter. I'm not sure if the trace would ever be useful or\n>> not, and I am mostly retaining it out of the logic of \"well, somebody\n>> bothered to write it\". I think the const issue has been there since\n>> e810e06357 (attr: tighten const correctness with git_attr and\n>> match_attr, 2017-01-27).\n>\n> And here's what that would look like.\n\nI highly suspect that I was the one who bothered, and while I admit\nit was useful while developing the attribute subsystem, I haven't\nneeded it for the past 10 or so years.\n\nSo unless there are some folks who want to throw everything into the\ntrace2 floodstream, I would prefer this alternative over the other\none.\n\nThanks.  All four patches look good.\n"},{"id":"464317","messageId":"fb4cfbd3-e738-f1e8-18f7-d4e90050b8b5@github.com","threadId":"58572","inReplyTo":"xmqqwn9clwba.fsf@gitster.g","subject":"Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-10-06T18:33:08Z","receivedAt":"2022-10-06T18:33:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/6/22 1:02 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:\n>>\n>>> The other obvious option is to just delete this debug code, and remove\n>>> the unused parameter. I'm not sure if the trace would ever be useful or\n>>> not, and I am mostly retaining it out of the logic of \"well, somebody\n>>> bothered to write it\". I think the const issue has been there since\n>>> e810e06357 (attr: tighten const correctness with git_attr and\n>>> match_attr, 2017-01-27).\n>>\n>> And here's what that would look like.\n> \n> I highly suspect that I was the one who bothered, and while I admit\n> it was useful while developing the attribute subsystem, I haven't\n> needed it for the past 10 or so years.\n> \n> So unless there are some folks who want to throw everything into the\n> trace2 floodstream, I would prefer this alternative over the other\n> one.\n\nAre you implying that you want to use the second version, that\ndeletes the information entirely? I'm leaning towards deleting\nit.\n\nIf not, and we should keep using traces, I do notice that the\noriginal version of the patch uses trace_printf_key() instead\nof a trace2 method. I think this is fine, too, since it's\nlikely only to be used by Git developers, who could look for\nwhich type of trace to use.\n\nThanks,\n-Stolee\n"},{"id":"464319","messageId":"xmqq1qrklr7p.fsf@gitster.g","threadId":"58572","inReplyTo":"fb4cfbd3-e738-f1e8-18f7-d4e90050b8b5@github.com","subject":"Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-06T18:52:42Z","receivedAt":"2022-10-06T18:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>> I highly suspect that I was the one who bothered, and while I admit\n>> it was useful while developing the attribute subsystem, I haven't\n>> needed it for the past 10 or so years.\n>> \n>> So unless there are some folks who want to throw everything into the\n>> trace2 floodstream, I would prefer this alternative over the other\n>> one.\n>\n> Are you implying that you want to use the second version, that\n> deletes the information entirely? I'm leaning towards deleting\n> it.\n\nSorry if I were not clear, but I would vote for using 4alt/4 and\nremove debugging code.  Unless there are folks who want to keep it,\nin which case I think trace2 is fine and I won't insist on removing\nwhat those folks, if any, want to keep.  Between trace and trace2,\nI do not have a strong opinion but if we were adding something new,\nwe would be adding to the latter?\n\n> If not, and we should keep using traces, I do notice that the\n> original version of the patch uses trace_printf_key() instead\n> of a trace2 method. I think this is fine, too, since it's\n> likely only to be used by Git developers, who could look for\n> which type of trace to use.\n>\n> Thanks,\n> -Stolee\n"},{"id":"464554","messageId":"Y0S3kKVm0jYxEXc1@coredump.intra.peff.net","threadId":"58572","inReplyTo":"xmqqwn9clwba.fsf@gitster.g","subject":"Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T00:23:44Z","receivedAt":"2022-10-11T00:23:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 06, 2022 at 10:02:33AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Thu, Oct 06, 2022 at 09:13:41AM -0400, Jeff King wrote:\n> >\n> >> The other obvious option is to just delete this debug code, and remove\n> >> the unused parameter. I'm not sure if the trace would ever be useful or\n> >> not, and I am mostly retaining it out of the logic of \"well, somebody\n> >> bothered to write it\". I think the const issue has been there since\n> >> e810e06357 (attr: tighten const correctness with git_attr and\n> >> match_attr, 2017-01-27).\n> >\n> > And here's what that would look like.\n> \n> I highly suspect that I was the one who bothered, and while I admit\n> it was useful while developing the attribute subsystem, I haven't\n> needed it for the past 10 or so years.\n\nIt was indeed you. :) I am quite happy to delete the code (via 4alt),\nbut didn't want to step on any toes.\n\n-Peff\n"},{"id":"464556","messageId":"Y0S4TWdeK0dSL3VZ@coredump.intra.peff.net","threadId":"58572","inReplyTo":"fb4cfbd3-e738-f1e8-18f7-d4e90050b8b5@github.com","subject":"Re: [PATCH 4alt/4] attr: drop DEBUG_ATTR code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T00:26:53Z","receivedAt":"2022-10-11T00:26:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 06, 2022 at 02:33:08PM -0400, Derrick Stolee wrote:\n\n> Are you implying that you want to use the second version, that\n> deletes the information entirely? I'm leaning towards deleting\n> it.\n> \n> If not, and we should keep using traces, I do notice that the\n> original version of the patch uses trace_printf_key() instead\n> of a trace2 method. I think this is fine, too, since it's\n> likely only to be used by Git developers, who could look for\n> which type of trace to use.\n\nI think it's moot, because we're all in agreement that the code should\nbe deleted. But...\n\n...it is not clear to me that GIT_TRACE_* is dead for further additions.\nI find it delightfully simple to use, and trace2 often rather baroque\nand complicated, since it is aimed more at structured data, as well as\nstoring or even transmitting data.\n\nI'm not sure if we have an overall plan there.\n\n-Peff\n"}]}