From patchwork Tue Mar 7 18:09:32 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164416 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 A0685C678DB for ; Tue, 7 Mar 2023 18:15:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232453AbjCGSPC (ORCPT ); Tue, 7 Mar 2023 13:15:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54224 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232354AbjCGSOf (ORCPT ); Tue, 7 Mar 2023 13:14:35 -0500 Received: from mail-ed1-x532.google.com (mail-ed1-x532.google.com [IPv6:2a00:1450:4864:20::532]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8E68C9EF7D for ; Tue, 7 Mar 2023 10:10:09 -0800 (PST) Received: by mail-ed1-x532.google.com with SMTP id a25so56174881edb.0 for ; Tue, 07 Mar 2023 10:10:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212602; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=1OOK10i7DgT28p7HLiKJC10k2AsXfqCe8TFpdOiB4yc=; b=kOwQiM40EDQl2WOlHoPbNwYjksXpj5qNtOPUpkmsyQRxU1zGTe4KSKO20mQiWZeO6Y olLpawSjVYbCG9ygHGh2p5O0iYoosLYjFGuRJ8srXa4bUb5IALasXBM1iUtxfEUZmJ0U BepAYDErVxwv5FlpWu2xK3EQY/tuhkZ3t9lx4wfzLdoSSdFSmGYqIT9LdjvI2VUcjnqT /qYT/P0ICgBn9S3LP9sSHodht1TuG/KtCKw1I4p1pEBfipY73pz5WrHh1rw/hSOBeaR8 tYqQxcaTK/hrZrqRzfQ7gECMNKXwj2lpgHFoIvaQ1s7wdR0vRufErWjQrLljBQezXfph lRng== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212602; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1OOK10i7DgT28p7HLiKJC10k2AsXfqCe8TFpdOiB4yc=; b=tzTuj1LAetkYfQxzVrfINL6lMzcTceR94I3sawU9GMzLsZKkTDOyWbsI5JQ1EAvHNd vsPEDLFPo0clfJsJ+UxNxlbaKeTddSHBatPjlvote8V9q5Pn9fY/Trhv0Mx3HoLhr3mN hY2TrF63TQaJY54XvtClbwvhIiTFooAC1quvzfzRiBcAMAebNz7rD8dUFvOhZptInlzw 0LCtcohjfkvs7wY+FZBjVKXvsfvAOADBIYiZsmAo7v+9j0wIxcgFFTNSLNlWDLjVmASB am1esO3F2Rk0leoHC+4UcJAxKffwLrHmkrb3dW3DWhReBrX7XFchwh+NBfd1ly88Y8IO DLGQ== X-Gm-Message-State: AO0yUKUcMTtaYYKIvj0e7ZRUzyyuQXcaysTEHpBvDhxi00eIKP21VR9r 9SPzM6xtRQLT4ArEsCtotBi/ouUQzIw3Xw== X-Google-Smtp-Source: AK7set+rcW9OV8zrkCncXQQX6QegkHLJauhDz98L3yUDCmJlGLYAi1qYFIPvsT8W+PDinyaRWZNeeg== X-Received: by 2002:a17:906:fe43:b0:88f:9c29:d232 with SMTP id wz3-20020a170906fe4300b0088f9c29d232mr21096070ejb.57.1678212601679; Tue, 07 Mar 2023 10:10:01 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:00 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 1/9] config tests: cover blind spots in git_die_config() tests Date: Tue, 7 Mar 2023 19:09:32 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org There were no tests checking for the output of the git_die_config() function in the config API, added in 5a80e97c827 (config: add `git_die_config()` to the config-set API, 2014-08-07). We only tested "test_must_fail", but didn't assert the output. We need tests for this because a subsequent commit will alter the return value of git_config_get_value_multi(), which is used to get the config values in the git_die_config() function. This test coverage helps to build confidence in that subsequent change. These tests cover different interactions with git_die_config(): - The "notes.mergeStrategy" test in "t/t3309-notes-merge-auto-resolve.sh" is a case where a function outside of config.c (git_config_get_notes_strategy()) calls git_die_config(). - The "gc.pruneExpire" test in "t5304-prune.sh" is a case where git_config_get_expiry() calls git_die_config(), covering a different "type" than the "string" test for "notes.mergeStrategy". - The "fetch.negotiationAlgorithm" test in "t/t5552-skipping-fetch-negotiator.sh" is a case where git_config_get_string*() calls git_die_config(). We also cover both the "from command-line config" and "in file..at line" cases here. The clobbering of existing ".git/config" files here is so that we're not implicitly testing the line count of the default config. Signed-off-by: Ævar Arnfjörð Bjarmason --- t/t3309-notes-merge-auto-resolve.sh | 7 ++++++- t/t5304-prune.sh | 12 ++++++++++-- t/t5552-skipping-fetch-negotiator.sh | 16 ++++++++++++++++ 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh index 141d3e4ca4d..9bd5dbf341f 100755 --- a/t/t3309-notes-merge-auto-resolve.sh +++ b/t/t3309-notes-merge-auto-resolve.sh @@ -360,7 +360,12 @@ test_expect_success 'merge z into y with invalid strategy => Fail/No changes' ' test_expect_success 'merge z into y with invalid configuration option => Fail/No changes' ' git config core.notesRef refs/notes/y && - test_must_fail git -c notes.mergeStrategy="foo" notes merge z && + cat >expect <<-\EOF && + error: unknown notes merge strategy foo + fatal: unable to parse '\''notes.mergeStrategy'\'' from command-line config + EOF + test_must_fail git -c notes.mergeStrategy="foo" notes merge z 2>actual && + test_cmp expect actual && # Verify no changes (y) verify_notes y y ' diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh index d65a5f94b4b..5500dd08426 100755 --- a/t/t5304-prune.sh +++ b/t/t5304-prune.sh @@ -72,8 +72,16 @@ test_expect_success 'gc: implicit prune --expire' ' ' test_expect_success 'gc: refuse to start with invalid gc.pruneExpire' ' - git config gc.pruneExpire invalid && - test_must_fail git gc + test_when_finished "rm -rf repo" && + git init repo && + >repo/.git/config && + git -C repo config gc.pruneExpire invalid && + cat >expect <<-\EOF && + error: Invalid gc.pruneexpire: '\''invalid'\'' + fatal: bad config variable '\''gc.pruneexpire'\'' in file '\''.git/config'\'' at line 2 + EOF + test_must_fail git -C repo gc 2>actual && + test_cmp expect actual ' test_expect_success 'gc: start with ok gc.pruneExpire' ' diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh index 165427d57e5..b55a9f65e6b 100755 --- a/t/t5552-skipping-fetch-negotiator.sh +++ b/t/t5552-skipping-fetch-negotiator.sh @@ -3,6 +3,22 @@ test_description='test skipping fetch negotiator' . ./test-lib.sh +test_expect_success 'fetch.negotiationalgorithm config' ' + test_when_finished "rm -rf repo" && + git init repo && + cat >repo/.git/config <<-\EOF && + [fetch] + negotiationAlgorithm + EOF + cat >expect <<-\EOF && + error: missing value for '\''fetch.negotiationalgorithm'\'' + fatal: bad config variable '\''fetch.negotiationalgorithm'\'' in file '\''.git/config'\'' at line 2 + EOF + test_expect_code 128 git -C repo fetch >out 2>actual && + test_must_be_empty out && + test_cmp expect actual +' + have_sent () { while test "$#" -ne 0 do From patchwork Tue Mar 7 18:09:33 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164424 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 91BE6C678D4 for ; Tue, 7 Mar 2023 18:16:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232561AbjCGSQw (ORCPT ); Tue, 7 Mar 2023 13:16:52 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55674 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232523AbjCGSOg (ORCPT ); Tue, 7 Mar 2023 13:14:36 -0500 Received: from mail-ed1-x536.google.com (mail-ed1-x536.google.com [IPv6:2a00:1450:4864:20::536]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1F2BBA188F for ; Tue, 7 Mar 2023 10:10:11 -0800 (PST) Received: by mail-ed1-x536.google.com with SMTP id da10so56022696edb.3 for ; Tue, 07 Mar 2023 10:10:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212603; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=4vhB/VvuxpEFLBgJp0BccsdhTjWfa6KSYpVrKo2IRgo=; b=lsqD8iB2W3HAKReyokFm4c/imIp3WTG6gboBGesgoJEhZz8iPeP8SbmOU4wb4FQsIY 46pBZWX9qIKWFLsbcTVf2U2kNWap9coaE0ZRdedi9i6knX9npl/M0sJLdfWKY+VRFG7S oIjLCRpKGzjy0luWo4ivN3lalVjJ9DXLBQGsCbwNDlktqi+11bnUjRGUgPdytXu3sEF7 pdu4jGpSDCbVjhD66eS6PvrGQ4CqNf2uv812suPh4n/2SAyZoKs3DuExD3N887G3i9Oq 0sbDslWshuZDELNPi5wi6RjVHibFbwiHebJPYgujVcAzxZOtOaPu0GN218eFV+D0C3VS dguQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212603; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4vhB/VvuxpEFLBgJp0BccsdhTjWfa6KSYpVrKo2IRgo=; b=MOvCN/QJvGnnduhwjlkvhMeyU69Zp35nxnopsr1HUzQ3wEZ3YwR7I0MtnTBqCiiFwL ELk++dC3eG3dhmQ0d3OzdPyzgvl/jgR+BppR9LDSKezrwm1+25BLb36imSe4jUbrV7B2 HcyD7LedYgoLreB0rJbTkQVx+wm4wikT//FpwZch15JP6pWsfqyt2ZuKnpa+PgEsDRdv ebhb/uFqpTNlzoa+KVtcVF9AUIBvDAguvkyFZCl+z9SOYfTcaaK+cTNQH0EBSuqG8bkT UG+9ldqLKLghz0t1/7/mgYZmLOlBW14Ii2Dwb/EvvzDXacOojHXlGu2OPghkMUk4U7FU rjMQ== X-Gm-Message-State: AO0yUKWO1gApLV2SWEPshJnn0RGAnlH6nUjgR0UzEq6RftYMcSpbz8Ve pihatZw7+QBnjCdhsQtgfRAjLNgLfhdZ0w== X-Google-Smtp-Source: AK7set+KsR1oQSTH3bQWgRfZiNJ7NBNfrh8Y3nXwt/Hxm/pKlNAmpDA2PYCtXh5yC0GTNMFx5sKnSA== X-Received: by 2002:a17:906:b751:b0:8f7:51e5:99d1 with SMTP id fx17-20020a170906b75100b008f751e599d1mr16538754ejb.37.1678212603107; Tue, 07 Mar 2023 10:10:03 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:02 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 2/9] config tests: add "NULL" tests for *_get_value_multi() Date: Tue, 7 Mar 2023 19:09:33 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org A less well known edge case in the config format is that keys can be value-less, a shorthand syntax for "true" boolean keys. I.e. these two are equivalent as far as "--type=bool" is concerned: [a]key [a]key = true But as far as our parser is concerned the values for these two are NULL, and "true". I.e. for a sequence like: [a]key=x [a]key [a]key=y We get a "struct string_list" with "string" members with ".string" values of: { "x", NULL, "y" } This behavior goes back to the initial implementation of git_config_bool() in 17712991a59 (Add ".git/config" file parser, 2005-10-10). When parts of the config_set API were tested for in [1] they didn't add coverage for 3/4 of the "(NULL)" cases handled in "t/helper/test-config.c". We'd test that case for "get_value", but not "get_value_multi", "configset_get_value" and "configset_get_value_multi". We now cover all of those cases, which in turn expose the details of how this part of the config API works. 1. 4c715ebb96a (test-config: add tests for the config_set API, 2014-07-28) Signed-off-by: Ævar Arnfjörð Bjarmason --- t/t1308-config-set.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index b38e158d3b2..4be1ab1147c 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -146,6 +146,71 @@ test_expect_success 'find multiple values' ' check_config get_value_multi case.baz sam bat hask ' +test_NULL_in_multi () { + local op="$1" && + local file="$2" && + + test_expect_success "$op: NULL value in config${file:+ in $file}" ' + config="$file" && + if test -z "$config" + then + config=.git/config && + test_when_finished "mv $config.old $config" && + mv "$config" "$config".old + fi && + + # Value-less in the middle of a list + cat >"$config" <<-\EOF && + [a]key=x + [a]key + [a]key=y + EOF + case "$op" in + *_multi) + cat >expect <<-\EOF + x + (NULL) + y + EOF + ;; + *) + cat >expect <<-\EOF + y + EOF + ;; + esac && + test-tool config "$op" a.key $file >actual && + test_cmp expect actual && + + # Value-less at the end of a least + cat >"$config" <<-\EOF && + [a]key=x + [a]key=y + [a]key + EOF + case "$op" in + *_multi) + cat >expect <<-\EOF + x + y + (NULL) + EOF + ;; + *) + cat >expect <<-\EOF + (NULL) + EOF + ;; + esac && + test-tool config "$op" a.key $file >actual && + test_cmp expect actual + ' +} + +test_NULL_in_multi "get_value_multi" +test_NULL_in_multi "configset_get_value" "my.config" +test_NULL_in_multi "configset_get_value_multi" "my.config" + test_expect_success 'find value from a configset' ' cat >config2 <<-\EOF && [case] From patchwork Tue Mar 7 18:09:34 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164419 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 21D4CC678D4 for ; Tue, 7 Mar 2023 18:15:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232513AbjCGSPL (ORCPT ); Tue, 7 Mar 2023 13:15:11 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57030 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232366AbjCGSOg (ORCPT ); Tue, 7 Mar 2023 13:14:36 -0500 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 48C11A2C3E for ; Tue, 7 Mar 2023 10:10:11 -0800 (PST) Received: by mail-ed1-x52a.google.com with SMTP id x3so55858646edb.10 for ; Tue, 07 Mar 2023 10:10:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212604; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=HxqCilXU7sHZvAh5hjU54F+iPbGbcFkVWeztk3YTZos=; b=ALpnKJlOEpgjkVbN1dy5SuGmmsm6FjG3zuMiBBCBnThMkrPQu8DmXZSzTgbhcU/F1F tWOidxmGMEAbPjCh8q1l9uLrRUS7zqM8IfCknkxQy7TSQKkHFsV4Q/mRNdu/Z+YGkWN+ bKbNe9aob9AXOtEyHYeZtglsCoYooVbJ/VBwPsgEXNslD4tf2C0zY3jqXq9+DLIcEotV U0hTVmebiIoDufu0oH/7T5OnH9UscBN3xgMiYvEgSOoZP8bArFtEHxkQ07SXZN+fRfzq ifKQecGADnZTMl76sqi0xE/YJJOUYFLMCcpzxmwSurYwyQmC7NPgeDnhyQmXKTAg0hVw V/mg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212604; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=HxqCilXU7sHZvAh5hjU54F+iPbGbcFkVWeztk3YTZos=; b=Xj1404heeDv3ka6JuOHACb9pigggUL35vq2MRL5HKpKcqf6SVXnr2oxsxeb9VlV/wv 4LzhOYBHj3VA6AU8ajs+R7BNa91PPP+22aAdO/We/KEh8I3G3A71Y3/WmQNe7SimGsOC +gJb7SQFXaKA8vTrRjidwJ+OUDJIEAZFDE6iFdDpjn4dgatKm0FMTZCvDpqWBi8GE3GX kzchHi/AL9D7MoAdPfcsjaKSrBy5Iur3HKqMIrhKt4CeKA7UpGdbWhUINni+8aLtki2N esEwYLCoyRA6Tu5Tb9aO9NcM9G1Hqd+WNhrFYEiGBf+Ldblme1mVAzZ6KJBU6sNxKujr DoTw== X-Gm-Message-State: AO0yUKWzcjQfNJGi03ILPxGKMmNGviW1qwYoNS6QjYeAZ4aGI5lial9r KK9Gq18Xxu9UlF1RCpqASVW3eDVvbMPtZw== X-Google-Smtp-Source: AK7set8qjPq0sgr0mJTyBXrk5+z45TAvcXEphmvCT9AObHc8nE62PQiiAtOYvRJs4fscsEKWNw2jcA== X-Received: by 2002:a17:907:20aa:b0:878:814d:bc99 with SMTP id pw10-20020a17090720aa00b00878814dbc99mr15205341ejb.66.1678212604336; Tue, 07 Mar 2023 10:10:04 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:03 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 3/9] config API: add and use a "git_config_get()" family of functions Date: Tue, 7 Mar 2023 19:09:34 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org We already have the basic "git_config_get_value()" function and its "repo_*" and "configset" siblings to get a given "key" and assign the last key found to a provided "value". But some callers don't care about that value, but just want to use the return value of the "get_value()" function to check whether the key exist (or another non-zero return value). The immediate motivation for this is that a subsequent commit will need to change all callers of the "*_get_value_multi()" family of functions. In two cases here we (ab)used it to check whether we had any values for the given key, but didn't care about the return value. The rest of the callers here used various other config API functions to do the same, all of which resolved to the same underlying functions to provide the answer. Some of these were using either git_config_get_string() or git_config_get_string_tmp(), see fe4c750fb13 (submodule--helper: fix a configure_added_submodule() leak, 2022-09-01) for a recent example. We can now use a helper function that doesn't require a throwaway variable. We could have changed git_configset_get_value_multi() (and then git_config_get_value() etc.) to accept a "NULL" as a "dest" for all callers, but let's avoid changing the behavior of existing API users. Having an "unused" value that we throw away internal to config.c is cheap. A "NULL as optional dest" pattern is also more fragile, as the intent of the caller might be misinterpreted if he were to accidentally pass "NULL", e.g. when "dest" is passed in from another function. Another name for this function could have been "*_config_key_exists()", as suggested in [1]. That would work for all of these callers, and would currently be equivalent to this function, as the git_configset_get_value() API normalizes all non-zero return values to a "1". But adding that API would set us up to lose information, as e.g. if git_config_parse_key() in the underlying configset_find_element() fails we'd like to return -1, not 1. Let's change the underlying configset_find_element() function to support this use-case, we'll make further use of it in a subsequent commit where the git_configset_get_value_multi() function itself will expose this new return value. This still leaves various inconsistencies and clobbering or ignoring of the return value in place. E.g here we're modifying configset_add_value(), but ever since it was added in [2] we've been ignoring its "int" return value, but as we're changing the configset_find_element() it uses, let's have it faithfully ferry that "ret" along. Let's also use the "RESULT_MUST_BE_USED" macro introduced in [3] to assert that we're checking the return value of configset_find_element(). We're leaving the same change to configset_add_value() for some future series. Once we start paying attention to its return value we'd need to ferry it up as deep as do_config_from(), and would need to make least read_{,very_}early_config() and git_protected_config() return an "int" instead of "void". Let's leave that for now, and focus on the *_get_*() functions. In a subsequent commit we'll fix the other *_get_*() functions to so that they'll ferry our underlying "ret" along, rather than normalizing it to a "return 1". But as an intermediate step to that we'll need to fix git_configset_get_value_multi() to return "int", and that change itself is smaller because of this change to migrate some callers away from the *_value_multi() API. 1. 3c8687a73ee (add `config_set` API for caching config-like files, 2014-07-28) 2. https://lore.kernel.org/git/xmqqczadkq9f.fsf@gitster.g/ 3. 1e8697b5c4e (submodule--helper: check repo{_submodule,}_init() return values, 2022-09-01), Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/gc.c | 5 +--- builtin/submodule--helper.c | 7 +++-- builtin/worktree.c | 3 +-- config.c | 51 ++++++++++++++++++++++++++++++++----- config.h | 18 +++++++++++++ t/helper/test-config.c | 22 ++++++++++++++++ t/t1308-config-set.sh | 43 ++++++++++++++++++++++++++++++- 7 files changed, 131 insertions(+), 18 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 02455fdcd73..e38d1783f30 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1493,7 +1493,6 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) }; int found = 0; const char *key = "maintenance.repo"; - char *config_value; char *maintpath = get_maintpath(); struct string_list_item *item; const struct string_list *list; @@ -1508,9 +1507,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) git_config_set("maintenance.auto", "false"); /* Set maintenance strategy, if unset */ - if (!git_config_get_string("maintenance.strategy", &config_value)) - free(config_value); - else + if (git_config_get("maintenance.strategy")) git_config_set("maintenance.strategy", "incremental"); list = git_config_get_value_multi(key); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 4c173d8b37a..2278e8c91cb 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -557,7 +557,7 @@ static int module_init(int argc, const char **argv, const char *prefix) * If there are no path args and submodule.active is set then, * by default, only initialize 'active' modules. */ - if (!argc && git_config_get_value_multi("submodule.active")) + if (!argc && !git_config_get("submodule.active")) module_list_active(&list); info.prefix = prefix; @@ -2743,7 +2743,7 @@ static int module_update(int argc, const char **argv, const char *prefix) * If there are no path args and submodule.active is set then, * by default, only initialize 'active' modules. */ - if (!argc && git_config_get_value_multi("submodule.active")) + if (!argc && !git_config_get("submodule.active")) module_list_active(&list); info.prefix = opt.prefix; @@ -3140,7 +3140,6 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con static void configure_added_submodule(struct add_data *add_data) { char *key; - const char *val; struct child_process add_submod = CHILD_PROCESS_INIT; struct child_process add_gitmodules = CHILD_PROCESS_INIT; @@ -3185,7 +3184,7 @@ static void configure_added_submodule(struct add_data *add_data) * is_submodule_active(), since that function needs to find * out the value of "submodule.active" again anyway. */ - if (!git_config_get_string_tmp("submodule.active", &val)) { + if (!git_config_get("submodule.active")) { /* * If the submodule being added isn't already covered by the * current configured pathspec, set the submodule's active flag diff --git a/builtin/worktree.c b/builtin/worktree.c index 254283aa6f5..2d81965711f 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -319,7 +319,6 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir) if (file_exists(from_file)) { struct config_set cs = { { 0 } }; - const char *core_worktree; int bare; if (safe_create_leading_directories(to_file) || @@ -338,7 +337,7 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir) to_file, "core.bare", NULL, "true", 0)) error(_("failed to unset '%s' in '%s'"), "core.bare", to_file); - if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) && + if (!git_configset_get(&cs, "core.worktree") && git_config_set_in_file_gently(to_file, "core.worktree", NULL)) error(_("failed to unset '%s' in '%s'"), diff --git a/config.c b/config.c index 00090a32fc3..d4f0e4fd619 100644 --- a/config.c +++ b/config.c @@ -2289,23 +2289,29 @@ void read_very_early_config(config_fn_t cb, void *data) config_with_options(cb, data, NULL, &opts); } -static struct config_set_element *configset_find_element(struct config_set *cs, const char *key) +RESULT_MUST_BE_USED +static int configset_find_element(struct config_set *cs, const char *key, + struct config_set_element **dest) { struct config_set_element k; struct config_set_element *found_entry; char *normalized_key; + int ret; + /* * `key` may come from the user, so normalize it before using it * for querying entries from the hashmap. */ - if (git_config_parse_key(key, &normalized_key, NULL)) - return NULL; + ret = git_config_parse_key(key, &normalized_key, NULL); + if (ret) + return ret; hashmap_entry_init(&k.ent, strhash(normalized_key)); k.key = normalized_key; found_entry = hashmap_get_entry(&cs->config_hash, &k, ent, NULL); free(normalized_key); - return found_entry; + *dest = found_entry; + return 0; } static int configset_add_value(struct config_set *cs, const char *key, const char *value) @@ -2314,8 +2320,11 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha struct string_list_item *si; struct configset_list_item *l_item; struct key_value_info *kv_info = xmalloc(sizeof(*kv_info)); + int ret; - e = configset_find_element(cs, key); + ret = configset_find_element(cs, key, &e); + if (ret) + return ret; /* * Since the keys are being fed by git_config*() callback mechanism, they * are already normalized. So simply add them without any further munging. @@ -2425,8 +2434,25 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char * const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key) { - struct config_set_element *e = configset_find_element(cs, key); - return e ? &e->value_list : NULL; + struct config_set_element *e; + + if (configset_find_element(cs, key, &e)) + return NULL; + else if (!e) + return NULL; + return &e->value_list; +} + +int git_configset_get(struct config_set *cs, const char *key) +{ + struct config_set_element *e; + int ret; + + if ((ret = configset_find_element(cs, key, &e))) + return ret; + else if (!e) + return 1; + return 0; } int git_configset_get_string(struct config_set *cs, const char *key, char **dest) @@ -2565,6 +2591,12 @@ void repo_config(struct repository *repo, config_fn_t fn, void *data) configset_iter(repo->config, fn, data); } +int repo_config_get(struct repository *repo, const char *key) +{ + git_config_check_init(repo); + return git_configset_get(repo->config, key); +} + int repo_config_get_value(struct repository *repo, const char *key, const char **value) { @@ -2679,6 +2711,11 @@ void git_config_clear(void) repo_config_clear(the_repository); } +int git_config_get(const char *key) +{ + return repo_config_get(the_repository, key); +} + int git_config_get_value(const char *key, const char **value) { return repo_config_get_value(the_repository, key, value); diff --git a/config.h b/config.h index 7606246531a..7dd62ca81bf 100644 --- a/config.h +++ b/config.h @@ -465,6 +465,9 @@ void git_configset_clear(struct config_set *cs); * value in the 'dest' pointer. */ +RESULT_MUST_BE_USED +int git_configset_get(struct config_set *cs, const char *key); + /* * Finds the highest-priority value for the configuration variable `key` * and config set `cs`, stores the pointer to it in `value` and returns 0. @@ -485,6 +488,14 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha /* Functions for reading a repository's config */ struct repository; void repo_config(struct repository *repo, config_fn_t fn, void *data); + +/** + * Run only the discover part of the repo_config_get_*() functions + * below, in addition to 1 if not found, returns negative values on + * error (e.g. if the key itself is invalid). + */ +RESULT_MUST_BE_USED +int repo_config_get(struct repository *repo, const char *key); int repo_config_get_value(struct repository *repo, const char *key, const char **value); const struct string_list *repo_config_get_value_multi(struct repository *repo, @@ -521,8 +532,15 @@ void git_protected_config(config_fn_t fn, void *data); * manner, the config API provides two functions `git_config_get_value` * and `git_config_get_value_multi`. They both read values from an internal * cache generated previously from reading the config files. + * + * For those git_config_get*() functions that aren't documented, + * consult the corresponding repo_config_get*() function's + * documentation. */ +RESULT_MUST_BE_USED +int git_config_get(const char *key); + /** * Finds the highest-priority value for the configuration variable `key`, * stores the pointer to it in `value` and returns 0. When the diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 4ba9eb65606..cbb33ae1fff 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -14,6 +14,8 @@ * get_value_multi -> prints all values for the entered key in increasing order * of priority * + * get -> print return value for the entered key + * * get_int -> print integer value for the entered key or die * * get_bool -> print bool value for the entered key or die @@ -109,6 +111,26 @@ int cmd__config(int argc, const char **argv) printf("Value not found for \"%s\"\n", argv[2]); goto exit1; } + } else if (argc == 3 && !strcmp(argv[1], "get")) { + int ret; + + if (!(ret = git_config_get(argv[2]))) + goto exit0; + else if (ret == 1) + printf("Value not found for \"%s\"\n", argv[2]); + else if (ret == -CONFIG_INVALID_KEY) + printf("Key \"%s\" is invalid\n", argv[2]); + else if (ret == -CONFIG_NO_SECTION_OR_NAME) + printf("Key \"%s\" has no section\n", argv[2]); + else + /* + * A normal caller should just check "ret < + * 0", but for our own tests let's BUG() if + * our whitelist of git_config_parse_key() + * return values isn't exhaustive. + */ + BUG("Key \"%s\" has unknown return %d", argv[2], ret); + goto exit1; } else if (argc == 3 && !strcmp(argv[1], "get_int")) { if (!git_config_get_int(argv[2], &val)) { printf("%d\n", val); diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index 4be1ab1147c..7def7053e1c 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -58,6 +58,8 @@ test_expect_success 'setup default config' ' skin = false nose = 1 horns + [value] + less EOF ' @@ -116,6 +118,45 @@ test_expect_success 'find value with the highest priority' ' check_config get_value case.baz "hask" ' +test_expect_success 'return value for an existing key' ' + test-tool config get lamb.chop >out 2>err && + test_must_be_empty out && + test_must_be_empty err +' + +test_expect_success 'return value for value-less key' ' + test-tool config get value.less >out 2>err && + test_must_be_empty out && + test_must_be_empty err +' + +test_expect_success 'return value for a missing key' ' + cat >expect <<-\EOF && + Value not found for "missing.key" + EOF + test_expect_code 1 test-tool config get missing.key >actual 2>err && + test_cmp actual expect && + test_must_be_empty err +' + +test_expect_success 'return value for a bad key: CONFIG_INVALID_KEY' ' + cat >expect <<-\EOF && + Key "fails.iskeychar.-" is invalid + EOF + test_expect_code 1 test-tool config get fails.iskeychar.- >actual 2>err && + test_cmp actual expect && + test_must_be_empty out +' + +test_expect_success 'return value for a bad key: CONFIG_NO_SECTION_OR_NAME' ' + cat >expect <<-\EOF && + Key "keynosection" has no section + EOF + test_expect_code 1 test-tool config get keynosection >actual 2>err && + test_cmp actual expect && + test_must_be_empty out +' + test_expect_success 'find integer value for a key' ' check_config get_int lamb.chop 65 ' @@ -272,7 +313,7 @@ test_expect_success 'proper error on error in default config files' ' cp .git/config .git/config.old && test_when_finished "mv .git/config.old .git/config" && echo "[" >>.git/config && - echo "fatal: bad config line 34 in file .git/config" >expect && + echo "fatal: bad config line 36 in file .git/config" >expect && test_expect_code 128 test-tool config get_value foo.bar 2>actual && test_cmp expect actual ' From patchwork Tue Mar 7 18:09:35 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164417 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 2CD82C6FD1A for ; Tue, 7 Mar 2023 18:15:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232506AbjCGSPF (ORCPT ); Tue, 7 Mar 2023 13:15:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35254 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232526AbjCGSOg (ORCPT ); Tue, 7 Mar 2023 13:14:36 -0500 Received: from mail-ed1-x533.google.com (mail-ed1-x533.google.com [IPv6:2a00:1450:4864:20::533]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 489E9A336F for ; Tue, 7 Mar 2023 10:10:12 -0800 (PST) Received: by mail-ed1-x533.google.com with SMTP id j11so36425806edq.4 for ; Tue, 07 Mar 2023 10:10:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212605; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=8pmRz3v3Hrht8hHKGL7OOBR85+GFIGz7ixvjtemVP/Y=; b=Y98foybvRMLZN0VYX0XllGRtYHiBP6xhC+dtnx2Z8FKH6YYVmeVoUVaX4T/uEf2iB3 M2UkbmMcy7anX5ldO7nBeCt3KaFBhx2AdcKZEto9GAnnebFbla5YQ02qogJLg21TmC3m xtzgIY6bHzyN3wDWYu4SRNyajP2U2OIpG0NxOP1ApyYYZeUmONzUo9d56Wv0YbeZC+bv VwLloFCCXoztWM2n6Sb+hXEKyRpqttvezYFj/rzS7j6bs6tiOFf0KI6fSpA36+cf3N8F YMZSGqR1jKzIya6aBMffJbon2Znn7ZD0HHykT6fQhdQrlyfdyGPhxYpdmnPTdJSBbAqS SxPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212605; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=8pmRz3v3Hrht8hHKGL7OOBR85+GFIGz7ixvjtemVP/Y=; b=8AOJYjlwoQaGrRECkUW2UVsahxVn8cS8iWspYZUQddHIlLkjgjLvebvenBECYfVApO pl0a6A05dVFPK/0fpwVikSwm+Sv/EBmHofFh8LjGR9693f10s0qY18Rco51w1L37Uvfl YOfoiVEdVmqevnT8GqbiarSwVMoV0NpmhNSPzr5NpmVuEwy6mXi7zlu7+VJvHjlXwLUj XxqJ28NMrgFNdsiCp8olGTRBhq6S8i6pLLrbmwmKMqOVwvn0txKerfKWHwZ0sFulwiqH Wdmcfx9X9OBCj9UIZvLc4kVlSHLSrzxWnVDsnGWI0L2+9c+NYTPoA52JzitYjJdja3hf gYHw== X-Gm-Message-State: AO0yUKXX39m/GXxZHlQ2opldkpil5efjhMTvfD1zt6EwepDhrC3akoGf lO+q4VTE8/81Gmgh9fiZumqaSHwpvO/hrA== X-Google-Smtp-Source: AK7set9c4zSV0dbP2tj3I2blWwcaqEO2CeScrjXcxoq+UXlf4updYNXXVzMYgPH4rYG9X53MyQviVw== X-Received: by 2002:aa7:dcd7:0:b0:4ac:b2c8:aeac with SMTP id w23-20020aa7dcd7000000b004acb2c8aeacmr13041990edu.40.1678212605585; Tue, 07 Mar 2023 10:10:05 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:04 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 4/9] versioncmp.c: refactor config reading next commit Date: Tue, 7 Mar 2023 19:09:35 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Refactor the reading of the versionSort.suffix and versionSort.prereleaseSuffix configuration variables to stay within the bounds of our CodingGuidelines when it comes to line length, and to avoid repeating ourselves. Renaming "deprecated_prereleases" to "oldl" doesn't help us to avoid line wrapping now, but it will in a subsequent commit. Let's also split out the names of the config variables into variables of our own, and refactor the nested if/else to avoid indenting it, and the existing bracing style issue. This all helps with the subsequent commit, where we'll need to start checking different git_config_get_value_multi() return value. See c026557a373 (versioncmp: generalize version sort suffix reordering, 2016-12-08) for the original implementation of most of this. Moving the "initialized = 1" assignment allows us to move some of this to the variable declarations in the subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason --- versioncmp.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/versioncmp.c b/versioncmp.c index 069ee94a4d7..323f5d35ea8 100644 --- a/versioncmp.c +++ b/versioncmp.c @@ -160,15 +160,18 @@ int versioncmp(const char *s1, const char *s2) } if (!initialized) { - const struct string_list *deprecated_prereleases; + const char *const newk = "versionsort.suffix"; + const char *const oldk = "versionsort.prereleasesuffix"; + const struct string_list *oldl; + + prereleases = git_config_get_value_multi(newk); + oldl = git_config_get_value_multi(oldk); + if (prereleases && oldl) + warning("ignoring %s because %s is set", oldk, newk); + else if (!prereleases) + prereleases = oldl; + initialized = 1; - prereleases = git_config_get_value_multi("versionsort.suffix"); - deprecated_prereleases = git_config_get_value_multi("versionsort.prereleasesuffix"); - if (prereleases) { - if (deprecated_prereleases) - warning("ignoring versionsort.prereleasesuffix because versionsort.suffix is set"); - } else - prereleases = deprecated_prereleases; } if (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1, &diff)) From patchwork Tue Mar 7 18:09:36 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164420 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 5191CC678D5 for ; Tue, 7 Mar 2023 18:15:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232519AbjCGSPO (ORCPT ); Tue, 7 Mar 2023 13:15:14 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57040 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232530AbjCGSOh (ORCPT ); Tue, 7 Mar 2023 13:14:37 -0500 Received: from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com [IPv6:2a00:1450:4864:20::52d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 09B8FA3B40 for ; Tue, 7 Mar 2023 10:10:13 -0800 (PST) Received: by mail-ed1-x52d.google.com with SMTP id u9so56063540edd.2 for ; Tue, 07 Mar 2023 10:10:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212607; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=auVZI/PFWr2hfsF3kAwrJYMzsfDjJzN0fkJNVnDKTpM=; b=keRqeXjJjwVIpb6bvAUjMJz+FNPIJBB049H+wzGzWxL8Ws8dWLr2xQYwXqshBCpXJy oVM2TX0xvHZXnoPE9SKzm/AK/sPKa7s5xmwgpnQEAlFBrzqUQelQKp/jRTrkCZNmcpRg +MqBLSXKkpzTJSZ1vdR2JWpGykmjdC2ilivFCFPCpxIistvySaYFbMRXXUWH73ts0l4e yk3xNs1euKhGwwA8AbKbLh00DPL8gY5wOxvcv0TRt5icI8rPkwYpKhj1k6FxdC4mr0r9 TfqqTudNkJgtzlrfOUf121KbCQ+uyX7Mru+H2Uln7qjcwJouI/8AEiwQgO+Qo4eGHDl3 yBGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212607; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=auVZI/PFWr2hfsF3kAwrJYMzsfDjJzN0fkJNVnDKTpM=; b=API6ZEop+4dBqh5Xjv0ocHWJAq8psYuA06bdUOS6H6x7+iv7IVtSadaak1BUGln2gC 45WTs31WCt32oyfU2URwrrPcdvf31egp6dl9ZvTnExJb1RBZ6LTEKtWpEDsu9RGbYOtB 2r2a0Y7AOnOVKUCWRjX0X2Ttkj0w88vJYOwpBxYGzXtsYioRnDy7qmILB7Hr0i1R/fGQ kSHp4jMZqs3Fb8OLEgZY2TLR9XseDHC5G0mMwBfEC4MKfPHpBNDdI8XSNztjrmgjO4ei otjrqUchS55/KwOIeru6tP7uS9px4qspoo+ivabYvIYMWPXyE3HinIO8mj8+4WnvaFlj Zpqg== X-Gm-Message-State: AO0yUKXcF/1oEoowxZo7FwODj2OCZtiWlkHj8QP5kMlaG8AtGuLVwiai ybdR+0FZBpaeKydcBKO9g2yXqW/voCV2mQ== X-Google-Smtp-Source: AK7set9x/5bR7W8F8+JNLOmaCRQBO5OL1O0b30EYyq8V103EovV5kVNmnqLxQekevpIMJICfweEGfQ== X-Received: by 2002:a17:906:68c5:b0:8b1:353a:2636 with SMTP id y5-20020a17090668c500b008b1353a2636mr13763365ejr.14.1678212606737; Tue, 07 Mar 2023 10:10:06 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:06 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 5/9] config API: have *_multi() return an "int" and take a "dest" Date: Tue, 7 Mar 2023 19:09:36 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Have the "git_configset_get_value_multi()" function and its siblings return an "int" and populate a "**dest" parameter like every other git_configset_get_*()" in the API. As we'll take advantage of in subsequent commits, this fixes a blind spot in the API where it wasn't possible to tell whether a list was empty from whether a config key existed. For now we don't make use of those new return values, but faithfully convert existing API users. Most of this is straightforward, commentary on cases that stand out: - To ensure that we'll properly use the return values of this function in the future we're using the "RESULT_MUST_BE_USED" macro introduced in [1]. As git_die_config() now has to handle this return value let's have it BUG() if it can't find the config entry. As tested for in a preceding commit we can rely on getting the config list in git_die_config(). - The loops after getting the "list" value in "builtin/gc.c" could also make use of "unsorted_string_list_has_string()" instead of using that loop, but let's leave that for now. - In "versioncmp.c" we now use the return value of the functions, instead of checking if the lists are still non-NULL. 1. 1e8697b5c4e (submodule--helper: check repo{_submodule,}_init() return values, 2022-09-01), Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/for-each-repo.c | 5 +---- builtin/gc.c | 10 ++++------ builtin/log.c | 6 +++--- config.c | 34 ++++++++++++++++++++-------------- config.h | 29 +++++++++++++++++++++-------- pack-bitmap.c | 6 +++++- submodule.c | 3 +-- t/helper/test-config.c | 6 ++---- versioncmp.c | 11 +++++++---- 9 files changed, 64 insertions(+), 46 deletions(-) diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c index 6aeac371488..fd0e7739e6a 100644 --- a/builtin/for-each-repo.c +++ b/builtin/for-each-repo.c @@ -45,14 +45,11 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) if (!config_key) die(_("missing --config=")); - values = repo_config_get_value_multi(the_repository, - config_key); - /* * Do nothing on an empty list, which is equivalent to the case * where the config variable does not exist at all. */ - if (!values) + if (repo_config_get_value_multi(the_repository, config_key, &values)) return 0; for (i = 0; !result && i < values->nr; i++) diff --git a/builtin/gc.c b/builtin/gc.c index e38d1783f30..2b3da377d52 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1510,8 +1510,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) if (git_config_get("maintenance.strategy")) git_config_set("maintenance.strategy", "incremental"); - list = git_config_get_value_multi(key); - if (list) { + if (!git_config_get_value_multi(key, &list)) { for_each_string_list_item(item, list) { if (!strcmp(maintpath, item->string)) { found = 1; @@ -1577,11 +1576,10 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi if (config_file) { git_configset_init(&cs); git_configset_add_file(&cs, config_file); - list = git_configset_get_value_multi(&cs, key); - } else { - list = git_config_get_value_multi(key); } - if (list) { + if (!(config_file + ? git_configset_get_value_multi(&cs, key, &list) + : git_config_get_value_multi(key, &list))) { for_each_string_list_item(item, list) { if (!strcmp(maintpath, item->string)) { found = 1; diff --git a/builtin/log.c b/builtin/log.c index a70fba198f9..e43f6f9d8c1 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -182,10 +182,10 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f int i; char *value = NULL; struct string_list *include = decoration_filter->include_ref_pattern; - const struct string_list *config_exclude = - git_config_get_value_multi("log.excludeDecoration"); + const struct string_list *config_exclude; - if (config_exclude) { + if (!git_config_get_value_multi("log.excludeDecoration", + &config_exclude)) { struct string_list_item *item; for_each_string_list_item(item, config_exclude) string_list_append(decoration_filter->exclude_ref_config_pattern, diff --git a/config.c b/config.c index d4f0e4fd619..569819b4a1b 100644 --- a/config.c +++ b/config.c @@ -2418,29 +2418,34 @@ int git_configset_add_file(struct config_set *cs, const char *filename) int git_configset_get_value(struct config_set *cs, const char *key, const char **value) { const struct string_list *values = NULL; + int ret; + /* * Follows "last one wins" semantic, i.e., if there are multiple matches for the * queried key in the files of the configset, the value returned will be the last * value in the value list for that key. */ - values = git_configset_get_value_multi(cs, key); + if ((ret = git_configset_get_value_multi(cs, key, &values))) + return ret; - if (!values) - return 1; assert(values->nr > 0); *value = values->items[values->nr - 1].string; return 0; } -const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key) +int git_configset_get_value_multi(struct config_set *cs, const char *key, + const struct string_list **dest) { struct config_set_element *e; + int ret; - if (configset_find_element(cs, key, &e)) - return NULL; + if ((ret = configset_find_element(cs, key, &e))) + return ret; else if (!e) - return NULL; - return &e->value_list; + return 1; + *dest = &e->value_list; + + return 0; } int git_configset_get(struct config_set *cs, const char *key) @@ -2604,11 +2609,11 @@ int repo_config_get_value(struct repository *repo, return git_configset_get_value(repo->config, key, value); } -const struct string_list *repo_config_get_value_multi(struct repository *repo, - const char *key) +int repo_config_get_value_multi(struct repository *repo, const char *key, + const struct string_list **dest) { git_config_check_init(repo); - return git_configset_get_value_multi(repo->config, key); + return git_configset_get_value_multi(repo->config, key, dest); } int repo_config_get_string(struct repository *repo, @@ -2721,9 +2726,9 @@ int git_config_get_value(const char *key, const char **value) return repo_config_get_value(the_repository, key, value); } -const struct string_list *git_config_get_value_multi(const char *key) +int git_config_get_value_multi(const char *key, const struct string_list **dest) { - return repo_config_get_value_multi(the_repository, key); + return repo_config_get_value_multi(the_repository, key, dest); } int git_config_get_string(const char *key, char **dest) @@ -2870,7 +2875,8 @@ void git_die_config(const char *key, const char *err, ...) error_fn(err, params); va_end(params); } - values = git_config_get_value_multi(key); + if (git_config_get_value_multi(key, &values)) + BUG("for key '%s' we must have a value to report on", key); kv_info = values->items[values->nr - 1].util; git_die_config_linenr(key, kv_info->filename, kv_info->linenr); } diff --git a/config.h b/config.h index 7dd62ca81bf..4db6b90ac20 100644 --- a/config.h +++ b/config.h @@ -450,10 +450,18 @@ int git_configset_add_file(struct config_set *cs, const char *filename); /** * Finds and returns the value list, sorted in order of increasing priority * for the configuration variable `key` and config set `cs`. When the - * configuration variable `key` is not found, returns NULL. The caller - * should not free or modify the returned pointer, as it is owned by the cache. + * configuration variable `key` is not found, returns 1 without touching + * `value`. + * + * The key will be parsed for validity with git_config_parse_key(), on + * error a negative value will be returned. + * + * The caller should not free or modify the returned pointer, as it is + * owned by the cache. */ -const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key); +RESULT_MUST_BE_USED +int git_configset_get_value_multi(struct config_set *cs, const char *key, + const struct string_list **dest); /** * Clears `config_set` structure, removes all saved variable-value pairs. @@ -498,8 +506,9 @@ RESULT_MUST_BE_USED int repo_config_get(struct repository *repo, const char *key); int repo_config_get_value(struct repository *repo, const char *key, const char **value); -const struct string_list *repo_config_get_value_multi(struct repository *repo, - const char *key); +RESULT_MUST_BE_USED +int repo_config_get_value_multi(struct repository *repo, const char *key, + const struct string_list **dest); int repo_config_get_string(struct repository *repo, const char *key, char **dest); int repo_config_get_string_tmp(struct repository *repo, @@ -553,10 +562,14 @@ int git_config_get_value(const char *key, const char **value); /** * Finds and returns the value list, sorted in order of increasing priority * for the configuration variable `key`. When the configuration variable - * `key` is not found, returns NULL. The caller should not free or modify - * the returned pointer, as it is owned by the cache. + * `key` is not found, returns 1 without touching `value`. + * + * The caller should not free or modify the returned pointer, as it is + * owned by the cache. */ -const struct string_list *git_config_get_value_multi(const char *key); +RESULT_MUST_BE_USED +int git_config_get_value_multi(const char *key, + const struct string_list **dest); /** * Resets and invalidates the config cache. diff --git a/pack-bitmap.c b/pack-bitmap.c index d2a42abf28c..15c5eb507c0 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -2314,7 +2314,11 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git) const struct string_list *bitmap_preferred_tips(struct repository *r) { - return repo_config_get_value_multi(r, "pack.preferbitmaptips"); + const struct string_list *dest; + + if (!repo_config_get_value_multi(r, "pack.preferbitmaptips", &dest)) + return dest; + return NULL; } int bitmap_is_preferred_refname(struct repository *r, const char *refname) diff --git a/submodule.c b/submodule.c index 3a0dfc417c0..4b6f5223b0c 100644 --- a/submodule.c +++ b/submodule.c @@ -274,8 +274,7 @@ int is_tree_submodule_active(struct repository *repo, free(key); /* submodule.active is set */ - sl = repo_config_get_value_multi(repo, "submodule.active"); - if (sl) { + if (!repo_config_get_value_multi(repo, "submodule.active", &sl)) { struct pathspec ps; struct strvec args = STRVEC_INIT; const struct string_list_item *item; diff --git a/t/helper/test-config.c b/t/helper/test-config.c index cbb33ae1fff..6dc4c37444f 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -97,8 +97,7 @@ int cmd__config(int argc, const char **argv) goto exit1; } } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { - strptr = git_config_get_value_multi(argv[2]); - if (strptr) { + if (!git_config_get_value_multi(argv[2], &strptr)) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; if (!v) @@ -181,8 +180,7 @@ int cmd__config(int argc, const char **argv) goto exit2; } } - strptr = git_configset_get_value_multi(&cs, argv[2]); - if (strptr) { + if (!git_configset_get_value_multi(&cs, argv[2], &strptr)) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; if (!v) diff --git a/versioncmp.c b/versioncmp.c index 323f5d35ea8..60c3a517122 100644 --- a/versioncmp.c +++ b/versioncmp.c @@ -162,13 +162,16 @@ int versioncmp(const char *s1, const char *s2) if (!initialized) { const char *const newk = "versionsort.suffix"; const char *const oldk = "versionsort.prereleasesuffix"; + const struct string_list *newl; const struct string_list *oldl; + int new = git_config_get_value_multi(newk, &newl); + int old = git_config_get_value_multi(oldk, &oldl); - prereleases = git_config_get_value_multi(newk); - oldl = git_config_get_value_multi(oldk); - if (prereleases && oldl) + if (!new && !old) warning("ignoring %s because %s is set", oldk, newk); - else if (!prereleases) + if (!new) + prereleases = newl; + else if (!old) prereleases = oldl; initialized = 1; From patchwork Tue Mar 7 18:09:37 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164418 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 24F65C678D5 for ; Tue, 7 Mar 2023 18:15:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232509AbjCGSPI (ORCPT ); Tue, 7 Mar 2023 13:15:08 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56952 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232528AbjCGSOg (ORCPT ); Tue, 7 Mar 2023 13:14:36 -0500 Received: from mail-ed1-x532.google.com (mail-ed1-x532.google.com [IPv6:2a00:1450:4864:20::532]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 05FD8A337F for ; Tue, 7 Mar 2023 10:10:13 -0800 (PST) Received: by mail-ed1-x532.google.com with SMTP id a25so56176023edb.0 for ; Tue, 07 Mar 2023 10:10:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212608; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=dDdnQ6yERIbX0aCwM5dihvL4YSD4XPKsQZRZTjX1yG4=; b=FUVJhl2j445ZEPsFsH8H+2Ex0wbxZsEi1QxEoisBwKy+SxVoYDWGj8SctdzM9ZXQe/ 6WoR9mU21HtG4ZBaKl3/ib4uXQKoSguG+RFFPmHTFf8sxSgAlRxPioTgmhj73mFgTAUZ KSrFxcQIMwIJW9IHH4wKIuho/A2hcKReI9ASiYjz7KhFkpscdAeMVNcB14qHCY46XC8J BVZyrGigUZNyb0cMpz/PIyZq1XZT3BC5EVZuGHLto4935dRM+uL8kwW59EdbZv/KtBXN jJVZkw2NUzd7uVWwiYs6hA43UsyNkLVgcAw7vYFSp30qHURGyMg2aEu6RIDqY/qRhRSo xuvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212608; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=dDdnQ6yERIbX0aCwM5dihvL4YSD4XPKsQZRZTjX1yG4=; b=0b/QYD1fECKeK4bXRH23yCjjgJeMFRsGcODy3vYGLZIDbj4FwCp37TDbodSpoz/UNr jalmOQ6rKZb7iIbdQfEZS2b3K7aosfyscGrcQGIVBiJ0/NPn9AKbVJFsdS2rmLleBEPc FXXoAkGZERqQ1m7x+hHJbN8RC2JDXO0SyTaueCZNM36OWCsFG9MGYDEjabLTPrcNmyz4 YygOh8ZoHLaohiwn1GbtVT3PnKog/jzIodOYqRpNv5rBtuD3zrx+zwKlE+r64V+PtF0G we6QvkZ/ibfXSUT9192okdTf6KSNz4bZAlVvPXSAKShIc1K6N+pKk3AYYTJveHqTED2y u3nA== X-Gm-Message-State: AO0yUKWePEOjGwpaLpSAkG0ix5W7ginhTiOkOvUriINAlQIoCMO5/ube iQSUJnszmJD7bSc3ls0DRXBMdVXe6vPMXA== X-Google-Smtp-Source: AK7set8WlqB/DME+ao01lrWYoDJuO5MZJsRmK08U64N+4YnGrDdEvnX+2TtReUSbS8tjFdZV3j51cQ== X-Received: by 2002:a17:907:8a22:b0:88d:697d:a3d2 with SMTP id sc34-20020a1709078a2200b0088d697da3d2mr18514643ejc.54.1678212607813; Tue, 07 Mar 2023 10:10:07 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:07 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 6/9] for-each-repo: error on bad --config Date: Tue, 7 Mar 2023 19:09:37 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org As noted in 6c62f015520 (for-each-repo: do nothing on empty config, 2021-01-08) this command wants to ignore a non-existing config key, but let's not conflate that with bad config. Before this, all these added tests would pass with an exit code of 0. We could preserve the comment added in 6c62f015520, but now that we're directly using the documented repo_config_get_value_multi() value it's just narrating something that should be obvious from the API use, so let's drop it. Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/for-each-repo.c | 11 ++++++----- t/t0068-for-each-repo.sh | 6 ++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c index fd0e7739e6a..224164addb3 100644 --- a/builtin/for-each-repo.c +++ b/builtin/for-each-repo.c @@ -32,6 +32,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) static const char *config_key = NULL; int i, result = 0; const struct string_list *values; + int err; const struct option options[] = { OPT_STRING(0, "config", &config_key, N_("config"), @@ -45,11 +46,11 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) if (!config_key) die(_("missing --config=")); - /* - * Do nothing on an empty list, which is equivalent to the case - * where the config variable does not exist at all. - */ - if (repo_config_get_value_multi(the_repository, config_key, &values)) + err = repo_config_get_value_multi(the_repository, config_key, &values); + if (err < 0) + usage_msg_optf(_("got bad config --config=%s"), + for_each_repo_usage, options, config_key); + else if (err) return 0; for (i = 0; !result && i < values->nr; i++) diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh index 3648d439a87..6b51e00da0e 100755 --- a/t/t0068-for-each-repo.sh +++ b/t/t0068-for-each-repo.sh @@ -40,4 +40,10 @@ test_expect_success 'do nothing on empty config' ' git for-each-repo --config=bogus.config -- help --no-such-option ' +test_expect_success 'error on bad config keys' ' + test_expect_code 129 git for-each-repo --config=a && + test_expect_code 129 git for-each-repo --config=a.b. && + test_expect_code 129 git for-each-repo --config="'\''.b" +' + test_done From patchwork Tue Mar 7 18:09:38 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164421 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 97E9BC678DB for ; Tue, 7 Mar 2023 18:15:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232525AbjCGSPR (ORCPT ); Tue, 7 Mar 2023 13:15:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56372 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232541AbjCGSOh (ORCPT ); Tue, 7 Mar 2023 13:14:37 -0500 Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [IPv6:2a00:1450:4864:20::52e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9403EA2C12 for ; Tue, 7 Mar 2023 10:10:13 -0800 (PST) Received: by mail-ed1-x52e.google.com with SMTP id cy23so55815936edb.12 for ; Tue, 07 Mar 2023 10:10:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212609; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=haGEaFXC8N6H13uRobelgkXdB/bRG8I9aJEAxY5PWo8=; b=i8WwBup6vFqZ9DsUSjpMr8rE0W3wQMWTNxCXMuUCHMwxsoK/2Bq56gf/XWo8cpnpzx dpUiuONcOySRbBdnqMWIuTmpl3Hd2Hne8BOWcqpmf3TB3venk1PvLVCJMzIQ3vMg5g2C BBNalpv1CyBTpEHyw8hnJwtvw/moVyZNzFpiuesUvwrWn7hxo6KpzFegW48Y/vtvqNPS 8XOz4WP1m+cAZDD7AbrF5kZNKAJORmRcoUt9mP1CulRmfMxqbS8ev2oSiRjUhsSWU/A9 4meo5llV7MYgnk1yvnZbqqFtEhNL6OkAjY4jHVorsxKdBTXtI/LcJSWvzvbLYQnJpguc s5Mw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212609; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=haGEaFXC8N6H13uRobelgkXdB/bRG8I9aJEAxY5PWo8=; b=CY82WzMHLyPArxcLLC5oShtDsxL2VtdVCKngAEIzIwJ7Y0rvdCDGWZL3BRbRXjGcIn ++F+slcrO4hNRrKBZDDPtgMCcpgTCkfrJH8fap+TzG7Ms4vbZ6huFXN1h1Xn8K8kDhD3 HfPVSttXKiCDAHf/OoyseSnhlSupVF2WjrWBKYl1s9Wk4DFzpCsXgROuZryZvx9quA8W kmFClXat3LTU9aWXARbm7YnsNbw3hyX1fYZS650iVUQsMCZKvYbqVJAzGd3okcwAj6nm qc22uyGmdw3hQDLHWNQ1XjYZHwwhfpnC0Jy3zd1i9/0ddrlbiThgZ0nNcmBqUIYBDVV/ HOkw== X-Gm-Message-State: AO0yUKVGdb78oNi7c3jdC2nxIMx5sWuvH7qF1R8Agsd4rGI3CZ7D0Jie rssUtsJ11xSza63UIxtEan9nf42ehHqS3w== X-Google-Smtp-Source: AK7set/khkH1BcDAigb1K1VgNbH4fNL41e6N51w+U9ZJx9JwZslecI6EHeA6/8zrAOi/WuVt3IeqKg== X-Received: by 2002:a17:906:b51:b0:8b1:3ba8:3f4d with SMTP id v17-20020a1709060b5100b008b13ba83f4dmr13734723ejg.70.1678212608913; Tue, 07 Mar 2023 10:10:08 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:08 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 7/9] config API users: test for *_get_value_multi() segfaults Date: Tue, 7 Mar 2023 19:09:38 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org As we'll discuss in the subsequent commit these tests all show *_get_value_multi() API users unable to handle there being a value-less key in the config, which is represented with a "NULL" for that entry in the "string" member of the returned "struct string_list", causing a segfault. These added tests exhaustively test for that issue, as we'll see in a subsequent commit we'll need to change all of the API users of *_get_value_multi(). These cases were discovered by triggering each one individually, and then adding these tests. Signed-off-by: Ævar Arnfjörð Bjarmason --- t/t4202-log.sh | 11 +++++++++++ t/t5310-pack-bitmaps.sh | 16 ++++++++++++++++ t/t7004-tag.sh | 12 ++++++++++++ t/t7413-submodule-is-active.sh | 12 ++++++++++++ t/t7900-maintenance.sh | 23 +++++++++++++++++++++++ 5 files changed, 74 insertions(+) diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 2ce2b41174d..e4f02d8208b 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -835,6 +835,17 @@ test_expect_success 'log.decorate configuration' ' ' +test_expect_failure 'parse log.excludeDecoration with no value' ' + cp .git/config .git/config.orig && + test_when_finished mv .git/config.orig .git/config && + + cat >>.git/config <<-\EOF && + [log] + excludeDecoration + EOF + git log --decorate=short +' + test_expect_success 'decorate-refs with glob' ' cat >expect.decorate <<-\EOF && Merge-tag-reach diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index 7d8dee41b0d..0306b399188 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -404,6 +404,22 @@ test_bitmap_cases () { ) ' + test_expect_failure 'pack.preferBitmapTips' ' + git init repo && + test_when_finished "rm -rf repo" && + ( + cd repo && + git config pack.writeBitmapLookupTable '"$writeLookupTable"' && + test_commit_bulk --message="%s" 103 && + + cat >>.git/config <<-\EOF && + [pack] + preferBitmapTips + EOF + git repack -adb + ) + ' + test_expect_success 'complains about multiple pack bitmaps' ' rm -fr repo && git init repo && diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 9aa1660651b..f343551a7d4 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1843,6 +1843,18 @@ test_expect_success 'invalid sort parameter in configuratoin' ' test_must_fail git tag -l "foo*" ' +test_expect_failure 'version sort handles empty value for versionsort.{prereleaseSuffix,suffix}' ' + cp .git/config .git/config.orig && + test_when_finished mv .git/config.orig .git/config && + + cat >>.git/config <<-\EOF && + [versionsort] + prereleaseSuffix + suffix + EOF + git tag -l --sort=version:refname +' + test_expect_success 'version sort with prerelease reordering' ' test_config versionsort.prereleaseSuffix -rc && git tag foo1.6-rc1 && diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh index 7cdc2637649..bfe27e50732 100755 --- a/t/t7413-submodule-is-active.sh +++ b/t/t7413-submodule-is-active.sh @@ -51,6 +51,18 @@ test_expect_success 'is-active works with submodule..active config' ' test-tool -C super submodule is-active sub1 ' +test_expect_failure 'is-active handles submodule.active config missing a value' ' + cp super/.git/config super/.git/config.orig && + test_when_finished mv super/.git/config.orig super/.git/config && + + cat >>super/.git/config <<-\EOF && + [submodule] + active + EOF + + test-tool -C super submodule is-active sub1 +' + test_expect_success 'is-active works with basic submodule.active config' ' test_when_finished "git -C super config submodule.sub1.URL ../sub" && test_when_finished "git -C super config --unset-all submodule.active" && diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 823331e44a0..d82eac6a471 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -524,6 +524,29 @@ test_expect_success 'register and unregister' ' git maintenance unregister --config-file ./other --force ' +test_expect_failure 'register with no value for maintenance.repo' ' + cp .git/config .git/config.orig && + test_when_finished mv .git/config.orig .git/config && + + cat >>.git/config <<-\EOF && + [maintenance] + repo + EOF + git maintenance register +' + +test_expect_failure 'unregister with no value for maintenance.repo' ' + cp .git/config .git/config.orig && + test_when_finished mv .git/config.orig .git/config && + + cat >>.git/config <<-\EOF && + [maintenance] + repo + EOF + git maintenance unregister && + git maintenance unregister --force +' + test_expect_success !MINGW 'register and unregister with regex metacharacters' ' META="a+b*c" && git init "$META" && From patchwork Tue Mar 7 18:09:39 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164422 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 EAC84C678D4 for ; Tue, 7 Mar 2023 18:15:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232530AbjCGSPT (ORCPT ); Tue, 7 Mar 2023 13:15:19 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57022 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232374AbjCGSOi (ORCPT ); Tue, 7 Mar 2023 13:14:38 -0500 Received: from mail-ed1-x529.google.com (mail-ed1-x529.google.com [IPv6:2a00:1450:4864:20::529]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A14ACA3B46 for ; Tue, 7 Mar 2023 10:10:13 -0800 (PST) Received: by mail-ed1-x529.google.com with SMTP id j11so36426674edq.4 for ; Tue, 07 Mar 2023 10:10:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212610; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=A2gzl/9erBVXLF++I+w8JZ1VhDWaU4UPE+k6lt7+N8U=; b=YgskkYQTlCga4FRoBwamwRvKQ1DgHrBxMAjEhouapPiHKxeLS+JMsxNSgTgee3Ekvz 7bZXM9GlLdxb9wfLgEo9kjdQ5m1jUBZTycWD7+Kl6WlAplPqsSanIhlYwmGmB3uNOlAf CiwlMiuCuT9TnUHOf89nIrzkrkacdJI7hdvU7kPrDrl4wulTcOSthcxTEmdZXbWChD5x MTABVW+D2991j5A/rtGubSKOUJG2ksPapwaZ/pZJzWhF6b0c/5rU6AhxVke5tIT9A+EE 5f7U+CbtYL2TXguxIqI6Ce5cGkoQdeWub/jr9jL+IgUSSA4JDVFeE8/KWUn/52L9dFiv sYOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212610; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=A2gzl/9erBVXLF++I+w8JZ1VhDWaU4UPE+k6lt7+N8U=; b=GLGrhsk5HlYq/yg8CBdu/efOTUIO1DMj8RV3sJJXKsHjXqouFNL5DN2LrQACsfHk7d dJMztC3msi2yPbnmSCHxsO1tcpJKTBVXtXKWFZitC6575u6pAjhX5/tqay+GJLvkbaIf G+qovrWt8rgaYhcgRew0jhxnmiJsWIVyGawLxthePZ7NphtUd5n7FWpj9NqoMYyhbWHx vY/nEXihjhmbijAxd2sD3fDvEKOp9m19Yz+xOO4BdULa+um3aRdLDc46v+DHueQ5vXJX WhnGQJ8M8mofsbqc/LwvTfBGv+PfSmwAruAqJjy6Wl7wmmWZpEktDZQC9pv1sG4edWYT OXZw== X-Gm-Message-State: AO0yUKUIRRi5gSzoC7tBsZd9A88AnI3YHerTe+nWgRdhq4dpABYqEIOu EGTB7xjlR91FcP5tWAdBxCHDQZLzT3keqw== X-Google-Smtp-Source: AK7set9hBwr7qJw0APiKRz5AG7yIutCbAHX7jgEAtyto+Ecc0hnUtQXx1zmvarYR+60i5y88ewmgpA== X-Received: by 2002:a17:907:3e88:b0:8ae:e724:ea15 with SMTP id hs8-20020a1709073e8800b008aee724ea15mr18707796ejc.76.1678212610257; Tue, 07 Mar 2023 10:10:10 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:09 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 8/9] config API: add "string" version of *_value_multi(), fix segfaults Date: Tue, 7 Mar 2023 19:09:39 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Fix numerous and mostly long-standing segfaults in consumers of the *_config_*value_multi() API. As discussed in the preceding commit an empty key in the config syntax yields a "NULL" string, which these users would give to strcmp() (or similar), resulting in segfaults. As this change shows, most users users of the *_config_*value_multi() API didn't really want such an an unsafe and low-level API, let's give them something with the safety of git_config_get_string() instead. This fix is similar to what the *_string() functions and others acquired in[1] and [2]. Namely introducing and using a safer "*_get_string_multi()" variant of the low-level "_*value_multi()" function. This fixes segfaults in code introduced in: - d811c8e17c6 (versionsort: support reorder prerelease suffixes, 2015-02-26) - c026557a373 (versioncmp: generalize version sort suffix reordering, 2016-12-08) - a086f921a72 (submodule: decouple url and submodule interest, 2017-03-17) - a6be5e6764a (log: add log.excludeDecoration config option, 2020-04-16) - 92156291ca8 (log: add default decoration filter, 2022-08-05) - 50a044f1e40 (gc: replace config subprocesses with API calls, 2022-09-27) There are now two users ofthe low-level API: - One in "builtin/for-each-repo.c", which we'll convert in a subsequent commit. - The "t/helper/test-config.c" code added in [3]. As seen in the preceding commit we need to give the "t/helper/test-config.c" caller these "NULL" entries. We could also alter the underlying git_configset_get_value_multi() function to be "string safe", but doing so would leave no room for other variants of "*_get_value_multi()" that coerce to other types. Such coercion can't be built on the string version, since as we've established "NULL" is a true value in the boolean context, but if we coerced it to "" for use in a list of strings it'll be subsequently coerced to "false" as a boolean. The callback pattern being used here will make it easy to introduce e.g. a "multi" variant which coerces its values to "bool", "int", "path" etc. 1. 40ea4ed9032 (Add config_error_nonbool() helper function, 2008-02-11) 2. 6c47d0e8f39 (config.c: guard config parser from value=NULL, 2008-02-11). 3. 4c715ebb96a (test-config: add tests for the config_set API, 2014-07-28) Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/gc.c | 6 +++--- builtin/log.c | 4 ++-- config.c | 32 ++++++++++++++++++++++++++++++++ config.h | 19 +++++++++++++++++++ pack-bitmap.c | 2 +- submodule.c | 2 +- t/t4202-log.sh | 8 ++++++-- t/t5310-pack-bitmaps.sh | 8 ++++++-- t/t7004-tag.sh | 9 +++++++-- t/t7413-submodule-is-active.sh | 8 ++++++-- t/t7900-maintenance.sh | 25 ++++++++++++++++++++----- versioncmp.c | 4 ++-- 12 files changed, 105 insertions(+), 22 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 2b3da377d52..9497bdf23e4 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1510,7 +1510,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) if (git_config_get("maintenance.strategy")) git_config_set("maintenance.strategy", "incremental"); - if (!git_config_get_value_multi(key, &list)) { + if (!git_config_get_string_multi(key, &list)) { for_each_string_list_item(item, list) { if (!strcmp(maintpath, item->string)) { found = 1; @@ -1578,8 +1578,8 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi git_configset_add_file(&cs, config_file); } if (!(config_file - ? git_configset_get_value_multi(&cs, key, &list) - : git_config_get_value_multi(key, &list))) { + ? git_configset_get_string_multi(&cs, key, &list) + : git_config_get_string_multi(key, &list))) { for_each_string_list_item(item, list) { if (!strcmp(maintpath, item->string)) { found = 1; diff --git a/builtin/log.c b/builtin/log.c index e43f6f9d8c1..ca847524fa4 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -184,8 +184,8 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f struct string_list *include = decoration_filter->include_ref_pattern; const struct string_list *config_exclude; - if (!git_config_get_value_multi("log.excludeDecoration", - &config_exclude)) { + if (!git_config_get_string_multi("log.excludeDecoration", + &config_exclude)) { struct string_list_item *item; for_each_string_list_item(item, config_exclude) string_list_append(decoration_filter->exclude_ref_config_pattern, diff --git a/config.c b/config.c index 569819b4a1b..c63034fb78b 100644 --- a/config.c +++ b/config.c @@ -2448,6 +2448,25 @@ int git_configset_get_value_multi(struct config_set *cs, const char *key, return 0; } +static int check_multi_string(struct string_list_item *item, void *util) +{ + return item->string ? 0 : config_error_nonbool(util); +} + +int git_configset_get_string_multi(struct config_set *cs, const char *key, + const struct string_list **dest) +{ + int ret; + + if ((ret = git_configset_get_value_multi(cs, key, dest))) + return ret; + if ((ret = for_each_string_list((struct string_list *)*dest, + check_multi_string, (void *)key))) + return ret; + + return 0; +} + int git_configset_get(struct config_set *cs, const char *key) { struct config_set_element *e; @@ -2616,6 +2635,13 @@ int repo_config_get_value_multi(struct repository *repo, const char *key, return git_configset_get_value_multi(repo->config, key, dest); } +int repo_config_get_string_multi(struct repository *repo, const char *key, + const struct string_list **dest) +{ + git_config_check_init(repo); + return git_configset_get_string_multi(repo->config, key, dest); +} + int repo_config_get_string(struct repository *repo, const char *key, char **dest) { @@ -2731,6 +2757,12 @@ int git_config_get_value_multi(const char *key, const struct string_list **dest) return repo_config_get_value_multi(the_repository, key, dest); } +int git_config_get_string_multi(const char *key, + const struct string_list **dest) +{ + return repo_config_get_string_multi(the_repository, key, dest); +} + int git_config_get_string(const char *key, char **dest) { return repo_config_get_string(the_repository, key, dest); diff --git a/config.h b/config.h index 4db6b90ac20..5f258e5b8df 100644 --- a/config.h +++ b/config.h @@ -463,6 +463,19 @@ RESULT_MUST_BE_USED int git_configset_get_value_multi(struct config_set *cs, const char *key, const struct string_list **dest); +/** + * A validation wrapper for git_configset_get_value_multi() which does + * for it what git_configset_get_string() does for + * git_configset_get_value(). + * + * The configuration syntax allows for "[section] key", which will + * give us a NULL entry in the "struct string_list", as opposed to + * "[section] key =" which is the empty string. Most users of the API + * are not prepared to handle NULL in a "struct string_list". + */ +int git_configset_get_string_multi(struct config_set *cs, const char *key, + const struct string_list **dest); + /** * Clears `config_set` structure, removes all saved variable-value pairs. */ @@ -509,6 +522,9 @@ int repo_config_get_value(struct repository *repo, RESULT_MUST_BE_USED int repo_config_get_value_multi(struct repository *repo, const char *key, const struct string_list **dest); +RESULT_MUST_BE_USED +int repo_config_get_string_multi(struct repository *repo, const char *key, + const struct string_list **dest); int repo_config_get_string(struct repository *repo, const char *key, char **dest); int repo_config_get_string_tmp(struct repository *repo, @@ -570,6 +586,9 @@ int git_config_get_value(const char *key, const char **value); RESULT_MUST_BE_USED int git_config_get_value_multi(const char *key, const struct string_list **dest); +RESULT_MUST_BE_USED +int git_config_get_string_multi(const char *key, + const struct string_list **dest); /** * Resets and invalidates the config cache. diff --git a/pack-bitmap.c b/pack-bitmap.c index 15c5eb507c0..d003c7e60b4 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -2316,7 +2316,7 @@ const struct string_list *bitmap_preferred_tips(struct repository *r) { const struct string_list *dest; - if (!repo_config_get_value_multi(r, "pack.preferbitmaptips", &dest)) + if (!repo_config_get_string_multi(r, "pack.preferbitmaptips", &dest)) return dest; return NULL; } diff --git a/submodule.c b/submodule.c index 4b6f5223b0c..30a103246ec 100644 --- a/submodule.c +++ b/submodule.c @@ -274,7 +274,7 @@ int is_tree_submodule_active(struct repository *repo, free(key); /* submodule.active is set */ - if (!repo_config_get_value_multi(repo, "submodule.active", &sl)) { + if (!repo_config_get_string_multi(repo, "submodule.active", &sl)) { struct pathspec ps; struct strvec args = STRVEC_INIT; const struct string_list_item *item; diff --git a/t/t4202-log.sh b/t/t4202-log.sh index e4f02d8208b..ae73aef922f 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -835,7 +835,7 @@ test_expect_success 'log.decorate configuration' ' ' -test_expect_failure 'parse log.excludeDecoration with no value' ' +test_expect_success 'parse log.excludeDecoration with no value' ' cp .git/config .git/config.orig && test_when_finished mv .git/config.orig .git/config && @@ -843,7 +843,11 @@ test_expect_failure 'parse log.excludeDecoration with no value' ' [log] excludeDecoration EOF - git log --decorate=short + cat >expect <<-\EOF && + error: missing value for '\''log.excludeDecoration'\'' + EOF + git log --decorate=short 2>actual && + test_cmp expect actual ' test_expect_success 'decorate-refs with glob' ' diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index 0306b399188..526a5a506eb 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh @@ -404,7 +404,7 @@ test_bitmap_cases () { ) ' - test_expect_failure 'pack.preferBitmapTips' ' + test_expect_success 'pack.preferBitmapTips' ' git init repo && test_when_finished "rm -rf repo" && ( @@ -416,7 +416,11 @@ test_bitmap_cases () { [pack] preferBitmapTips EOF - git repack -adb + cat >expect <<-\EOF && + error: missing value for '\''pack.preferbitmaptips'\'' + EOF + git repack -adb 2>actual && + test_cmp expect actual ) ' diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index f343551a7d4..f4a31ada79a 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1843,7 +1843,7 @@ test_expect_success 'invalid sort parameter in configuratoin' ' test_must_fail git tag -l "foo*" ' -test_expect_failure 'version sort handles empty value for versionsort.{prereleaseSuffix,suffix}' ' +test_expect_success 'version sort handles empty value for versionsort.{prereleaseSuffix,suffix}' ' cp .git/config .git/config.orig && test_when_finished mv .git/config.orig .git/config && @@ -1852,7 +1852,12 @@ test_expect_failure 'version sort handles empty value for versionsort.{prereleas prereleaseSuffix suffix EOF - git tag -l --sort=version:refname + cat >expect <<-\EOF && + error: missing value for '\''versionsort.suffix'\'' + error: missing value for '\''versionsort.prereleasesuffix'\'' + EOF + git tag -l --sort=version:refname 2>actual && + test_cmp expect actual ' test_expect_success 'version sort with prerelease reordering' ' diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh index bfe27e50732..887d181b72e 100755 --- a/t/t7413-submodule-is-active.sh +++ b/t/t7413-submodule-is-active.sh @@ -51,7 +51,7 @@ test_expect_success 'is-active works with submodule..active config' ' test-tool -C super submodule is-active sub1 ' -test_expect_failure 'is-active handles submodule.active config missing a value' ' +test_expect_success 'is-active handles submodule.active config missing a value' ' cp super/.git/config super/.git/config.orig && test_when_finished mv super/.git/config.orig super/.git/config && @@ -60,7 +60,11 @@ test_expect_failure 'is-active handles submodule.active config missing a value' active EOF - test-tool -C super submodule is-active sub1 + cat >expect <<-\EOF && + error: missing value for '\''submodule.active'\'' + EOF + test-tool -C super submodule is-active sub1 2>actual && + test_cmp expect actual ' test_expect_success 'is-active works with basic submodule.active config' ' diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index d82eac6a471..487e326b3fa 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -524,7 +524,7 @@ test_expect_success 'register and unregister' ' git maintenance unregister --config-file ./other --force ' -test_expect_failure 'register with no value for maintenance.repo' ' +test_expect_success 'register with no value for maintenance.repo' ' cp .git/config .git/config.orig && test_when_finished mv .git/config.orig .git/config && @@ -532,10 +532,15 @@ test_expect_failure 'register with no value for maintenance.repo' ' [maintenance] repo EOF - git maintenance register + cat >expect <<-\EOF && + error: missing value for '\''maintenance.repo'\'' + EOF + git maintenance register 2>actual && + test_cmp expect actual && + git config maintenance.repo ' -test_expect_failure 'unregister with no value for maintenance.repo' ' +test_expect_success 'unregister with no value for maintenance.repo' ' cp .git/config .git/config.orig && test_when_finished mv .git/config.orig .git/config && @@ -543,8 +548,18 @@ test_expect_failure 'unregister with no value for maintenance.repo' ' [maintenance] repo EOF - git maintenance unregister && - git maintenance unregister --force + cat >expect <<-\EOF && + error: missing value for '\''maintenance.repo'\'' + EOF + test_expect_code 128 git maintenance unregister 2>actual.raw && + grep ^error actual.raw >actual && + test_cmp expect actual && + git config maintenance.repo && + + git maintenance unregister --force 2>actual.raw && + grep ^error actual.raw >actual && + test_cmp expect actual && + git config maintenance.repo ' test_expect_success !MINGW 'register and unregister with regex metacharacters' ' diff --git a/versioncmp.c b/versioncmp.c index 60c3a517122..7498da96e0e 100644 --- a/versioncmp.c +++ b/versioncmp.c @@ -164,8 +164,8 @@ int versioncmp(const char *s1, const char *s2) const char *const oldk = "versionsort.prereleasesuffix"; const struct string_list *newl; const struct string_list *oldl; - int new = git_config_get_value_multi(newk, &newl); - int old = git_config_get_value_multi(oldk, &oldl); + int new = git_config_get_string_multi(newk, &newl); + int old = git_config_get_string_multi(oldk, &oldl); if (!new && !old) warning("ignoring %s because %s is set", oldk, newk); From patchwork Tue Mar 7 18:09:40 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= X-Patchwork-Id: 13164423 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 0B205C678DB for ; Tue, 7 Mar 2023 18:15:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232541AbjCGSPW (ORCPT ); Tue, 7 Mar 2023 13:15:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56384 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232543AbjCGSOh (ORCPT ); Tue, 7 Mar 2023 13:14:37 -0500 Received: from mail-ed1-x532.google.com (mail-ed1-x532.google.com [IPv6:2a00:1450:4864:20::532]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C7FB6A3B4B for ; Tue, 7 Mar 2023 10:10:13 -0800 (PST) Received: by mail-ed1-x532.google.com with SMTP id k10so32046125edk.13 for ; Tue, 07 Mar 2023 10:10:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1678212611; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=MF0PYtt5n9HWgHVoGWzMnk9xS8YKsx0IF17BQ+px9dU=; b=BK+CjjVmXRPDJUO5+yCc3GZZ9PsH0dYfKQJ4GFh3b0MT47pYbkYamGo1V+vzO9ZDkO jZ3aP3vbhSXbOiZxFOMPSV/I+x1we4a69DlIEhlPfWtK/q8ccfp+HeiUtGily5ZwdRv4 nW0awltHYDAHgiPmdoHg5v/36OmB7mDEM+/XFnC0tioHz+1PRPCx5N2cZZeQ/eUcYh7G RiswH7m8sgOUc47ZBfVjlpCgWFyS343yGqBUDQtP2RhmuuU0xtWoHgp9MUheM+MyyKfg SkKD1uk9QHC5bsUclvKv7LeP9W/YaVJL9nr1Js2t+Ga7gnTbvknJFdTp7jzQgqNeaFW8 OSQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678212611; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=MF0PYtt5n9HWgHVoGWzMnk9xS8YKsx0IF17BQ+px9dU=; b=FhcxmINal6KRGHL2mgjpH4JDpZrwiQtTW01qInNkZJ1a74rawDxHNiHKskh1hqdP1G DTNYI4+Eiis7sDnJyW9YXCA7vVR4Vsf5zL/NHdBMMLZD4vdPg73tRXJ5qB8FpZhl5T0s IZWJiQhTpkQIo0xqn8v515nRvBTc+S4hTXXyDaV5NrldtYtlF+91srh8EyExSVrVAZKX uTDUKNZxsuBGrC2EtMUo0tt2A+aH1RcKabUbT6oElQSwg2/d0hm4X17V3Wr03QK1QzwL 4Esu5SXnMztQ4omDAf6OJsY6uHRN2CFoX7tDBEYNlFIjsynQzpeHsxOqvppsP31Bk0RK nqmg== X-Gm-Message-State: AO0yUKWm5rukEPbwYPrRawbBGHBfVaiCE5TbScMnhndEIMr25AweQjmM 4k0km7iDqDW4gqz2w92xrce2VXjZS1lbWw== X-Google-Smtp-Source: AK7set9czJJhnkmJBALtgs8ETHhKcM6dMOqZMbzJWXmR20MTT4x4TrkEaYpCVEuoB58Ogx2/KvEeRg== X-Received: by 2002:a17:906:35d5:b0:8f8:1501:be85 with SMTP id p21-20020a17090635d500b008f81501be85mr14422759ejb.34.1678212611284; Tue, 07 Mar 2023 10:10:11 -0800 (PST) Received: from vm.nix.is (vm.nix.is. [2a01:4f8:120:2468::2]) by smtp.gmail.com with ESMTPSA id d5-20020a1709063ec500b008b1797a53b4sm6401008ejj.215.2023.03.07.10.10.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Mar 2023 10:10:10 -0800 (PST) From: =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= To: git@vger.kernel.org Cc: Junio C Hamano , Derrick Stolee , Elijah Newren , Jeff King , Taylor Blau , =?utf-8?q?SZEDER_?= =?utf-8?q?G=C3=A1bor?= , Glen Choo , Calvin Wan , Emily Shaffer , raymond@heliax.dev, zweiss@equinix.com, =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Subject: [PATCH v6 9/9] for-each-repo: with bad config, don't conflate and Date: Tue, 7 Mar 2023 19:09:40 +0100 Message-Id: X-Mailer: git-send-email 2.40.0.rc1.1034.g5867a1b10c5 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Fix a logic error in 4950b2a2b5c (for-each-repo: run subcommands on configured repos, 2020-09-11). Due to assuming that elements returned from the repo_config_get_value_multi() call wouldn't be "NULL" we'd conflate the and part of the argument list when running commands. As noted in the preceding commit the fix is to move to a safer "*_string_multi()" version of the *_multi() API. This change is separated from the rest because those all segfaulted. In this change we ended up with different behavior. When using the "--config=" form we take each element of the list as a path to a repository. E.g. with a configuration like: [repo] list = /some/repo We would, with this command: git for-each-repo --config=repo.list status builtin Run a "git status" in /some/repo, as: git -C /some/repo status builtin I.e. ask "status" to report on the "builtin" directory. But since a configuration such as this would result in a "struct string_list *" with one element, whose "string" member is "NULL": [repo] list We would, when constructing our command-line in "builtin/for-each-repo.c"... strvec_pushl(&child.args, "-C", path, NULL); for (i = 0; i < argc; i++) strvec_push(&child.args, argv[i]); ...have that "path" be "NULL", and as strvec_pushl() stops when it sees NULL we'd end with the first "argv" element as the argument to the "-C" option, e.g.: git -C status builtin I.e. we'd run the command "builtin" in the "status" directory. In another context this might be an interesting security vulnerability, but I think that this amounts to a nothingburger on that front. A hypothetical attacker would need to be able to write config for the victim to run, if they're able to do that there's more interesting attack vectors. See the "safe.directory" facility added in 8d1a7448206 (setup.c: create `safe.bareRepository`, 2022-07-14). An even more unlikely possibility would be an attacker able to generate the config used for "for-each-repo --config=", but nothing else (e.g. an automated system producing that list). Even in that case the attack vector is limited to the user running commands whose name matches a directory that's interesting to the attacker (e.g. a "log" directory in a repository). The second argument (if any) of the command is likely to make git die without doing anything interesting (e.g. "-p" to "log", there being no "-p" built-in command to run). Signed-off-by: Ævar Arnfjörð Bjarmason --- builtin/for-each-repo.c | 2 +- t/t0068-for-each-repo.sh | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c index 224164addb3..ce8f7a99086 100644 --- a/builtin/for-each-repo.c +++ b/builtin/for-each-repo.c @@ -46,7 +46,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix) if (!config_key) die(_("missing --config=")); - err = repo_config_get_value_multi(the_repository, config_key, &values); + err = repo_config_get_string_multi(the_repository, config_key, &values); if (err < 0) usage_msg_optf(_("got bad config --config=%s"), for_each_repo_usage, options, config_key); diff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh index 6b51e00da0e..4b90b74d5d5 100755 --- a/t/t0068-for-each-repo.sh +++ b/t/t0068-for-each-repo.sh @@ -46,4 +46,17 @@ test_expect_success 'error on bad config keys' ' test_expect_code 129 git for-each-repo --config="'\''.b" ' +test_expect_success 'error on NULL value for config keys' ' + cat >>.git/config <<-\EOF && + [empty] + key + EOF + cat >expect <<-\EOF && + error: missing value for '\''empty.key'\'' + EOF + test_expect_code 129 git for-each-repo --config=empty.key 2>actual.raw && + grep ^error actual.raw >actual && + test_cmp expect actual +' + test_done