From 6823c19888a5d1b68da725bf2093dc1155a50afb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 6 Oct 2022 09:10:15 -0400 Subject: [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 Signed-off-by: Junio C Hamano --- 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 }, From 7faba18a9a82b32aeacc5dd5f525764a80640a95 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 6 Oct 2022 09:10:53 -0400 Subject: [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 Signed-off-by: Junio C Hamano --- 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; } From 116761ba9cdee81e0d7b4671f14f9bd256f2cb36 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 6 Oct 2022 09:11:31 -0400 Subject: [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 Signed-off-by: Junio C Hamano --- 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")), From 69c5f17f11a2afe6b56b04f5a4377e4332858cde Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 6 Oct 2022 09:23:19 -0400 Subject: [PATCH 4/4] 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 Signed-off-by: Junio C Hamano --- 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; }