git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Rudy Rigot via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Jeff Hostetler" <git@jeffhostetler.com>,
	"Taylor Blau" <me@ttaylorr.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Rudy Rigot" <rudy.rigot@gmail.com>
Subject: Re: [PATCH v6] status: long status advice adapted to recent capabilities
Date: Mon, 21 Nov 2022 00:06:10 -0500	[thread overview]
Message-ID: <CAPig+cRPQ7bmG6+U+oQGGUFiSiHoMMpMk8FDJ7GMJvwCXifa9g@mail.gmail.com> (raw)
In-Reply-To: <pull.1384.v6.git.1668547188070.gitgitgadget@gmail.com>

On Tue, Nov 15, 2022 at 4:19 PM Rudy Rigot via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Improve the advice displayed when `git status` is slow because
> of excessive numbers of untracked files.  Update the `git status`
> man page to explain the various configuration options.
>
> Signed-off-by: Rudy Rigot <rudy.rigot@gmail.com>
> ---
>     Here is version 6 for this patch.
>
>     Changes since v5:
>
>      * End of sentence for fsmonitor case was changed to "but the results
>        were cached, and subsequent runs may be faster."
>      * Except for that, mostly style and doc fixes.

Thanks, I think v6 addresses all my earlier review comments. Just one
or two notes below...

> +test_expect_success setup '
> +       git checkout -b test &&
> +       cat >.gitignore <<-\EOF &&
> +       /actual
> +       /expected
> +       /out
> +       EOF
> +       git add .gitignore &&
> +       git commit -m "Add .gitignore"
> +'

I suppose I wasn't paying close enough attention earlier, but I see
now that this is creating a branch named "test". If I remove the `git
checkout -b test` line entirely and change "test" to "master" in the
"expected" files, all the tests pass just fine. So, it's not apparent
why you need to create a specially-named branch here rather than
simply accepting the default branch name. The reason I bring this up
is that unnecessary code, whether in tests or elsewhere, can confuse
future readers (and reviewers, such as myself) into wondering if
something subtle is going on which the reader is overlooking. (This is
very much the same sort of concern as when I asked in an earlier
review why the conditions in an `if` got switched around from the way
they were in the original code; such unnecessary changes can confuse
future readers.)

If the answer is that you wanted to avoid the term "master", then an
alternative would have been to override the default branch name at the
top of the script:

    GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
    export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

That said, this is minor, and I'm not keen on eating up more of your
time or reviewer time, so I doubt this is worth a reroll.

> +test_expect_success 'when core.untrackedCache and fsmonitor are unset' '
> +       test_might_fail git config --unset-all core.untrackedCache &&
> +       test_might_fail git config --unset-all core.fsmonitor &&

When I suggested the above code, it had slipped my mind that we have a
test_unconfig() function in t/test-lib-functions.sh which does this
more concisely:

    test_unconfig core.untrackedCache &&
    test_unconfig core.fsmonitor &&

But what you have here in v6 is good enough; it's not worth wasting
your time or reviewer time rerolling just to make this change. I
mention it merely for future reference.

  reply	other threads:[~2022-11-21  5:06 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-15 13:04 [PATCH] fsmonitor: long status advice adapted to the fsmonitor use case Rudy Rigot via GitGitGadget
2022-10-15 13:08 ` Rudy Rigot
2022-10-17 15:39 ` Jeff Hostetler
2022-10-17 16:59   ` Rudy Rigot
2022-10-20 12:56     ` Jeff Hostetler
2022-10-20 20:17       ` Rudy Rigot
2022-10-24 14:55         ` Jeff Hostetler
2022-10-29  0:06 ` [PATCH v2] status: long status advice adapted to recent capabilities Rudy Rigot via GitGitGadget
2022-11-02 19:45   ` Jeff Hostetler
2022-11-02 20:34     ` Rudy Rigot
2022-11-02 23:59     ` Taylor Blau
2022-11-03 14:28       ` Rudy Rigot
2022-11-04  8:52         ` Ævar Arnfjörð Bjarmason
2022-11-04 15:33           ` Rudy Rigot
2022-11-04 21:38         ` Taylor Blau
2022-11-02 21:27   ` [PATCH v3] " Rudy Rigot via GitGitGadget
2022-11-04 21:40     ` Taylor Blau
2022-11-07 20:02       ` Derrick Stolee
2022-11-07 23:19         ` Taylor Blau
2022-11-15 16:38       ` Jeff Hostetler
2022-11-07 20:01     ` Derrick Stolee
2022-11-07 20:20       ` Eric Sunshine
2022-11-07 20:31         ` Rudy Rigot
2022-11-10  4:46     ` [PATCH v4] " Rudy Rigot via GitGitGadget
2022-11-10  5:42       ` Eric Sunshine
2022-11-10 17:01         ` Rudy Rigot
2022-11-10 17:30           ` Eric Sunshine
2022-11-10 17:47             ` Rudy Rigot
2022-11-10 20:04       ` [PATCH v5] " Rudy Rigot via GitGitGadget
2022-11-15 16:39         ` Jeff Hostetler
2022-11-15 16:42           ` Rudy Rigot
2022-11-15 17:26         ` Eric Sunshine
2022-11-15 17:45           ` Rudy Rigot
2022-11-15 18:06             ` Eric Sunshine
2022-11-15 18:08               ` Rudy Rigot
2022-11-15 21:19         ` [PATCH v6] " Rudy Rigot via GitGitGadget
2022-11-21  5:06           ` Eric Sunshine [this message]
2022-11-21 15:54             ` Rudy Rigot
2022-11-21 16:17               ` Eric Sunshine
2022-11-22 16:52                 ` Rudy Rigot
2022-11-22 17:18                   ` Eric Sunshine
2022-11-22 17:24                     ` Eric Sunshine
2022-11-22 17:29                       ` Rudy Rigot
2022-11-22 17:40                     ` Eric Sunshine
2022-11-22 18:07                       ` Eric Sunshine
2022-11-22 19:19                         ` Rudy Rigot
2022-11-22 19:48                           ` Eric Sunshine
2022-11-22 16:59           ` [PATCH v7] status: modernize git-status "slow untracked files" advice Rudy Rigot via GitGitGadget
2022-11-22 22:07             ` [PATCH v8] " Rudy Rigot via GitGitGadget
2022-11-25  4:58               ` Junio C Hamano
2022-11-29 15:21                 ` Rudy Rigot
2022-11-30  0:51                   ` Rudy Rigot
2022-11-30  0:52               ` [PATCH v9] " Rudy Rigot via GitGitGadget
2022-12-01  6:48                 ` Junio C Hamano
2022-12-01 15:16                   ` Rudy Rigot
2022-12-01 22:45                     ` Junio C Hamano
2022-12-01 22:57                       ` Rudy Rigot
2023-05-11  5:17               ` [PATCH v8] " Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cRPQ7bmG6+U+oQGGUFiSiHoMMpMk8FDJ7GMJvwCXifa9g@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=rudy.rigot@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).