From patchwork Thu Oct 6 13:10:15 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13000300 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C6A4BC4332F for ; Thu, 6 Oct 2022 13:10:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229496AbiJFNKh (ORCPT ); Thu, 6 Oct 2022 09:10:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58378 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231393AbiJFNKS (ORCPT ); Thu, 6 Oct 2022 09:10:18 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AD41D5581 for ; Thu, 6 Oct 2022 06:10:16 -0700 (PDT) Received: (qmail 11989 invoked by uid 109); 6 Oct 2022 13:10:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 06 Oct 2022 13:10:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26198 invoked by uid 111); 6 Oct 2022 13:10:15 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 06 Oct 2022 09:10:15 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 6 Oct 2022 09:10:15 -0400 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 1/4] test-submodule: inline resolve_relative_url() function Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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 patchwork Thu Oct 6 13:10:53 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13000301 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C0163C433F5 for ; Thu, 6 Oct 2022 13:11:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230516AbiJFNLD (ORCPT ); Thu, 6 Oct 2022 09:11:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59356 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231251AbiJFNKz (ORCPT ); Thu, 6 Oct 2022 09:10:55 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9F775A599C for ; Thu, 6 Oct 2022 06:10:54 -0700 (PDT) Received: (qmail 11994 invoked by uid 109); 6 Oct 2022 13:10:53 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 06 Oct 2022 13:10:53 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26204 invoked by uid 111); 6 Oct 2022 13:10:53 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 06 Oct 2022 09:10:53 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 6 Oct 2022 09:10:53 -0400 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 2/4] multi-pack-index: avoid writing to global in option callback Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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 patchwork Thu Oct 6 13:11:31 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13000302 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 041A0C433FE for ; Thu, 6 Oct 2022 13:11:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230285AbiJFNLg (ORCPT ); Thu, 6 Oct 2022 09:11:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35648 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230089AbiJFNLf (ORCPT ); Thu, 6 Oct 2022 09:11:35 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5E3F42F66E for ; Thu, 6 Oct 2022 06:11:33 -0700 (PDT) Received: (qmail 11997 invoked by uid 109); 6 Oct 2022 13:11:32 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 06 Oct 2022 13:11:32 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26209 invoked by uid 111); 6 Oct 2022 13:11:32 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 06 Oct 2022 09:11:32 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 6 Oct 2022 09:11:31 -0400 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 3/4] commit: avoid writing to global in option callback Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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 patchwork Thu Oct 6 13:13:41 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13000306 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3204BC433FE for ; Thu, 6 Oct 2022 13:13:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230469AbiJFNNo (ORCPT ); Thu, 6 Oct 2022 09:13:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36678 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229496AbiJFNNn (ORCPT ); Thu, 6 Oct 2022 09:13:43 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ADC39A59A2 for ; Thu, 6 Oct 2022 06:13:42 -0700 (PDT) Received: (qmail 12000 invoked by uid 109); 6 Oct 2022 13:13:42 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 06 Oct 2022 13:13:42 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 26214 invoked by uid 111); 6 Oct 2022 13:13:41 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 06 Oct 2022 09:13:41 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 6 Oct 2022 09:13:41 -0400 From: Jeff King To: git@vger.kernel.org Subject: [PATCH 4/4] attr: convert DEBUG_ATTR to use trace API Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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);