From patchwork Tue Jun 6 14:20:18 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Derrick Stolee X-Patchwork-Id: 13269260 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 8912DC77B73 for ; Tue, 6 Jun 2023 14:20:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234856AbjFFOUY (ORCPT ); Tue, 6 Jun 2023 10:20:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55484 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231883AbjFFOUX (ORCPT ); Tue, 6 Jun 2023 10:20:23 -0400 Received: from mail-wm1-x330.google.com (mail-wm1-x330.google.com [IPv6:2a00:1450:4864:20::330]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0590F139 for ; Tue, 6 Jun 2023 07:20:22 -0700 (PDT) Received: by mail-wm1-x330.google.com with SMTP id 5b1f17b1804b1-3f6da07feb2so61623185e9.0 for ; Tue, 06 Jun 2023 07:20:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686061220; x=1688653220; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:from:to:cc:subject:date :message-id:reply-to; bh=CUbr/Joz5JtKw8l7ovCakoxOWUFuxb2GxmbdziE2S6g=; b=LT/Vt9k/f4MHkNh94bknPs2Dh4uMOplGNKi26HhDfnLRzzlQA4NuexmBgpRKRmyeux nTM730PLW/gfaNgQxKoz/hhR5l/fgY1z96Dkh+JHvHONlspI1EAYeuEvuX/PgnjT+Zbq i1qzsx7e5VFeLI+4sfVNqZsc2TF2n7vmgzz6aFQoS+MbgZ/+Jt9UcTNaifUZWNrbdbgK g3s6/Ey/XbOxRcq91EFJqUsKRu8KbVy4fy6ggLNpDwS6m14xTioxJ7eHiCqMHMTBbkl1 4AgwucDECNX080ZX1Ir+3zJpnLzE3kxcVClAmpUjxyvyIQbVGI/hKe3oWVPbEWQtLZAH fvjQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686061220; x=1688653220; h=cc:to:mime-version:content-transfer-encoding:fcc:subject:date:from :references:in-reply-to:message-id:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=CUbr/Joz5JtKw8l7ovCakoxOWUFuxb2GxmbdziE2S6g=; b=EZHdwBO29F5AATj+FU3NNe56lRyCbQRpDbneq/hivBZlsU41RbKGfLVWfzxQYaynFL 7iWiFiz0c2HjZKKe2uE19mAc4o7H/YptwKS9n91G0zh6gU58eDkAG2yi3whyvTci89QP Uk2lHF1ap2qJ9xHEJ6XSl174fWzKsQDhrc8eWVxrpmAM4hVZiQCEgb8HHyoLCpBeNdZP v/I7dEZy85XidedcZLKNHUC18qLEHXBWFFUxx1JKFzsOOMQ59Uapm7X1NuP/oy5/7Oy9 knVNvf+FUxvCRKola0kbeqVS8tGpC5Aaf9skgKSUUwzD+rJCcFNf4Va6BqoQQqtSSN/4 UZdQ== X-Gm-Message-State: AC+VfDwmOkmYt7iPn4KPrmGwsFvkz6gpX0z7lWXIuP5j177BA/7j0a6N 3IQsUWQd2khRLX43D/NMIu1ZppyqKZE= X-Google-Smtp-Source: ACHHUZ6W0785PfheZkYLQv1xkwA/4xYO2uxTdFYU0bK8g/zxY/ch7VKJMA7vcf9HoIOGIbWdY2BpKg== X-Received: by 2002:a05:600c:2947:b0:3f6:1377:8b15 with SMTP id n7-20020a05600c294700b003f613778b15mr2251897wmd.21.1686061220080; Tue, 06 Jun 2023 07:20:20 -0700 (PDT) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id z9-20020a5d4c89000000b002fe96f0b3acsm12799287wrs.63.2023.06.06.07.20.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Jun 2023 07:20:19 -0700 (PDT) Message-Id: In-Reply-To: References: Date: Tue, 06 Jun 2023 14:20:18 +0000 Subject: [PATCH v2] add: check color.ui for interactive add Fcc: Sent MIME-Version: 1.0 To: git@vger.kernel.org Cc: gitster@pobox.com, johannes.schindelin@gmx.de, Jeff King , Derrick Stolee , Derrick Stolee Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From: Derrick Stolee From: Derrick Stolee When 'git add -i' and 'git add -p' were converted to a builtin, they introduced a color bug: the 'color.ui' config setting is ignored. The included test demonstrates an example that is similar to the previous test, which focuses on customizing colors. Here, we are demonstrating that colors are not being used at all by comparing the raw output and the color-decoded version of that output. The fix is simple, to use git_color_default_config() as the fallback for git_add_config(). A more robust change would instead encapsulate the git_use_color_default global in methods that would check the config setting if it has not been initialized yet. Some ideas are being discussed on this front [1], but nothing has been finalized. [1] https://lore.kernel.org/git/pull.1539.git.1685716420.gitgitgadget@gmail.com/ This test case naturally bisects down to 0527ccb1b55 (add -i: default to the built-in implementation, 2021-11-30), but the fix makes it clear that this would be broken even if we added the config to use the builtin earlier than this. Reported-by: Greg Alexander Signed-off-by: Derrick Stolee --- add: check color.ui for interactive add This was reported by Greg Alexander gitgreg@galexander.org during Git IRC Standup [2]. [2] https://colabti.org/irclogger/irclogger_log/git-devel?date=2023-06-05 This is also a reoccurrence of the "config not loaded" bug from [3]. [3] https://lore.kernel.org/git/pull.1530.git.1683745654800.gitgitgadget@gmail.com/ I linked above to my RFC on lazy-loading global Git config, and these are the same "root cause" (not loading something early enough in the process) and my RFC proposes to fix this by changing our access patterns. By encapsulating these globals, we can make sure they are initialized from config before they are accessed. But that's a discussion for another thread. For now, fix the bug and we'll worry about the "better" (and bigger) thing to do another time. Update in v2 ============ * The test is simplified to compare the raw output to the color-decoded output. This makes the test more robust to possible future changes to interactive add. Thanks, -Stolee P.S. This fails the whitespace check due to the necessary left-padding spaces in the expected output in the test file. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1541%2Fderrickstolee%2Fadd-interactive-color-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1541/derrickstolee/add-interactive-color-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1541 Range-diff vs v1: 1: a76893de8a7 ! 1: 6d3e34d51c5 add: check color.ui for interactive add @@ Commit message The included test demonstrates an example that is similar to the previous test, which focuses on customizing colors. Here, we are - demonstrating that colors are not being used at all. + demonstrating that colors are not being used at all by comparing the raw + output and the color-decoded version of that output. The fix is simple, to use git_color_default_config() as the fallback for git_add_config(). A more robust change would instead encapsulate the @@ t/t3701-add-interactive.sh: test_expect_success 'colors can be overridden' ' + -c color.ui=false \ + add -i >actual.raw actual && -+ cat >expect <<-\EOF && -+ staged unstaged path -+ 1: +3/-0 +2/-1 color-test -+ -+ *** Commands *** -+ 1: [s]tatus 2: [u]pdate 3: [r]evert 4: [a]dd untracked -+ 5: [p]atch 6: [d]iff 7: [q]uit 8: [h]elp -+ What now> status - show paths with changes -+ update - add working tree state to the staged set of changes -+ revert - revert staged set of changes back to the HEAD version -+ patch - pick hunks and update selectively -+ diff - view diff between HEAD and index -+ add untracked - add contents of untracked files to the staged set of changes -+ *** Commands *** -+ 1: [s]tatus 2: [u]pdate 3: [r]evert 4: [a]dd untracked -+ 5: [p]atch 6: [d]iff 7: [q]uit 8: [h]elp -+ What now> Bye. -+ EOF -+ test_cmp expect actual ++ test_cmp actual.raw actual +' + test_expect_success 'colorized diffs respect diff.wsErrorHighlight' ' builtin/add.c | 2 +- t/t3701-add-interactive.sh | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09 diff --git a/builtin/add.c b/builtin/add.c index 76cc026a68a..6137e7b4ad7 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -365,7 +365,7 @@ static int add_config(const char *var, const char *value, void *cb) return 0; } - return git_default_config(var, value, cb); + return git_color_default_config(var, value, cb); } static const char embedded_advice[] = N_( diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 3982b6b49dc..a93fe54e2ad 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -734,6 +734,21 @@ test_expect_success 'colors can be overridden' ' test_cmp expect actual ' +test_expect_success 'colors can be skipped with color.ui=false' ' + git reset --hard && + test_when_finished "git rm -f color-test" && + test_write_lines context old more-context >color-test && + git add color-test && + test_write_lines context new more-context another-one >color-test && + + test_write_lines help quit >input && + force_color git \ + -c color.ui=false \ + add -i >actual.raw actual && + test_cmp actual.raw actual +' + test_expect_success 'colorized diffs respect diff.wsErrorHighlight' ' git reset --hard && From patchwork Wed Jun 7 13:44:48 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Derrick Stolee X-Patchwork-Id: 13270719 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 DDD3FC77B7A for ; Wed, 7 Jun 2023 13:45:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235463AbjFGNpI (ORCPT ); Wed, 7 Jun 2023 09:45:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59962 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240817AbjFGNox (ORCPT ); Wed, 7 Jun 2023 09:44:53 -0400 Received: from mail-yw1-x112d.google.com (mail-yw1-x112d.google.com [IPv6:2607:f8b0:4864:20::112d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5EBE31993 for ; Wed, 7 Jun 2023 06:44:52 -0700 (PDT) Received: by mail-yw1-x112d.google.com with SMTP id 00721157ae682-565ee3d14c2so82131617b3.2 for ; Wed, 07 Jun 2023 06:44:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=google; t=1686145490; x=1688737490; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=fhXmptONXfKQG4/azc/n5esg6bpY2sbIHHxi+gxlcSU=; b=T6EZDisfnDBs3i/T/ZeQeGwuFkkU9/neeydzjoadqcTTmCDOWf3DmHoPEGkxWTo81t zjRuGabvmSpJDb5HDlai76GdEH+lJY4ulJuzGwj2nABIPaUTU4sZUSG0BCXevJTR453K GJK/fFlLk7+wfyxi4JByI1d4v1DhIB2/I1d/V0kWUyxRt12egbz63wAn80UzBWMyp1ER XByabGgoh/gDprb8LKGVq9q1C+jSFZM3rz/mE9B7qFvMIej1rDRbVQzfOSodVWv7tqXT Q5CgpzayOAcWRMRW4hEYUO2tMC2kLi210QOrziciHjFSeigwLc1LdiEwCTgfXegNkiat SF8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686145490; x=1688737490; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=fhXmptONXfKQG4/azc/n5esg6bpY2sbIHHxi+gxlcSU=; b=OYiuH+YnduK3XH1IVI6lFuhyOR2mSzyflu6DunDuLFucNTEL29VD9ohtstU3/kflBt zqQFz57oAZIVfCTHFygQ3o+zEO1ueyc7zYmm3v92huljAlEyd2NgrI59ZHEXoCzqOt9Y V7nYLwPPKJxfq3Y6y5VQgdUjlrhLbf0wlWrB0BJedkDYLlWFj3dcHu0d+FNNZnAStQru IC+CkxMpytHiszg5JGbC4Pb891u5ZC/IhjnzqHtOqkQelq4vpc0SPnbFL1POfJ/fogeH l4+TdVa8IHBQGVfdP1r3PnIAJzrsscK3R09YeSAHrNm6q/PpyVXeVFs+XUn1J5r0bDCd km1Q== X-Gm-Message-State: AC+VfDxGGktadf/zumbPhKm3uKf6/Mi1yMwd20GxZrJJpO4WmGav+pud Sfl1e27Vh8baodrjsBCS9IHv X-Google-Smtp-Source: ACHHUZ6F0Jni8hgM4DlUAbC58FM7lsdm/hWEUCaSKmmjcbvvLZdoiSXLEWbwquKOlkvxBy0WtBRAUQ== X-Received: by 2002:a81:83cb:0:b0:569:f208:a120 with SMTP id t194-20020a8183cb000000b00569f208a120mr7001030ywf.38.1686145490381; Wed, 07 Jun 2023 06:44:50 -0700 (PDT) Received: from ?IPV6:2600:1700:e72:80a0:ec69:1775:6713:a647? ([2600:1700:e72:80a0:ec69:1775:6713:a647]) by smtp.gmail.com with ESMTPSA id g185-20020a0df6c2000000b00545a08184fdsm4720626ywf.141.2023.06.07.06.44.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 07 Jun 2023 06:44:50 -0700 (PDT) Message-ID: <281431b8-af40-9de9-f4b4-c596c5dbb3af@github.com> Date: Wed, 7 Jun 2023 09:44:48 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.2 Subject: [Patch v2 2/2] add: test use of brackets when color is disabled Content-Language: en-US To: Derrick Stolee via GitGitGadget , git@vger.kernel.org Cc: gitster@pobox.com, johannes.schindelin@gmx.de, Jeff King , Phillip Wood References: From: Derrick Stolee In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From 02156b81bbb2cafb19d702c55d45714fcf224048 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 7 Jun 2023 09:39:01 -0400 Subject: [PATCH v2 2/2] add: test use of brackets when color is disabled The interactive add command, 'git add -i', displays a menu of options using full words. When color is enabled, the first letter of each word is changed to a highlight color to signal that the first letter could be used as a command. Without color, brackets ("[]") are used around these first letters. This behavior was not previously tested directly in t3701, so add a test for it now. Since we use 'git add -i >actual Signed-off-by: Derrick Stolee --- Here is the patch to add this test on top of the previous change. I've only validated this on my local computer, not through the GitGitGadget PR. If needed, I could send a v3 via GitGitGadget, but thought this would be a simple-enough addition here. Thanks, -Stolee t/t3701-add-interactive.sh | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index a93fe54e2ad..df3e85fc8d6 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -734,6 +734,29 @@ test_expect_success 'colors can be overridden' ' test_cmp expect actual ' +test_expect_success 'brackets appear without color' ' + git reset --hard && + test_when_finished "git rm -f bracket-test" && + test_write_lines context old more-context >bracket-test && + git add bracket-test && + test_write_lines context new more-context another-one >bracket-test && + + test_write_lines quit >input && + git add -i >actual expect <<-\EOF && + | staged unstaged path + | 1: +3/-0 +2/-1 bracket-test + | + |*** Commands *** + | 1: [s]tatus 2: [u]pdate 3: [r]evert 4: [a]dd untracked + | 5: [p]atch 6: [d]iff 7: [q]uit 8: [h]elp + |What now> Bye. + EOF + + test_cmp expect actual +' + test_expect_success 'colors can be skipped with color.ui=false' ' git reset --hard && test_when_finished "git rm -f color-test" &&