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 <rudy.rigot@gmail.com>
Cc: "Rudy Rigot via GitGitGadget" <gitgitgadget@gmail.com>,
	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>
Subject: Re: [PATCH v6] status: long status advice adapted to recent capabilities
Date: Mon, 21 Nov 2022 11:17:36 -0500	[thread overview]
Message-ID: <CAPig+cQgu=i6pZTzoNYGZ_6X=DGdmwa=dPhSQVqD+eLCZCGJSg@mail.gmail.com> (raw)
In-Reply-To: <CANaDLWJM1VRivm8VLqxg+w8K-+49E0km6AgOzWzN9X=TgzaEiA@mail.gmail.com>

On Mon, Nov 21, 2022 at 10:55 AM Rudy Rigot <rudy.rigot@gmail.com> wrote:
> > 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.
>
> Eh, there's nothing wrong with striving for perfection. Lemme do one
> more reroll...

Reviewer time is a scarce resource on the mailing list these days,
which is why I'm hesitant to see rerolls for minor or subjective
changes. However, if you're going to reroll anyhow, I have a couple
more things to say... (below)

> > 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 was that it failed some CI pipelines before I did this,
> with some pipelines printing "main" instead of "master" into the git
> status output. I fixed it right away, so I don't know if it was a CI
> glitch that day or if it would still be the same running it now. I
> could have redacted the branch name away from the output, but it
> seemed simpler and more readable to just set the branch name in stone
> for all pipelines.

Most likely it wasn't a glitch, but rather (I'd guess) that Windows CI
uses "main" already, whereas Unix CI's still use "master".

> > an alternative would have been to override the default branch name at the
> > top of the script:
>
> Oh, this seems like a better way to do what I was trying to do. I'll
> change it now.
>
> > we have a test_unconfig() function
>
> I'll use that.
>
> New patch coming!

If you're going to reroll, then I'll mention a couple more things
which I held back before since I want to use reviewer time wisely.
Nevertheless...

First, having the commit message explain the problem first and then
the solution is more reviewer-friendly, not the solution and then the
problem as this patch is doing. Additionally, the commit message
should be written in imperative mood. Documentation/SubmittingPatches
has a good discussion of these points. It's also typically unnecessary
for the commit message to say that the patch is adding new tests;
reviewers assume that you will do so when appropriate, and the patch
itself shows plainly enough that you did. Taking these points into
consideration, you might write the commit message like this:

    status: modernize git-status "slow untracked files" advice

    `git status` can be slow when there are a large number of
    untracked files and directories since Git must search the entire
    worktree to enumerate them.  When it is too slow, Git prints
    advice with the elapsed search time and a suggestion to disable
    the search using the `-uno` option.  This suggestion also carries
    a warning that might scare off some users.

    However, these days, `-uno` isn't the only option.  Git can reduce
    the size and time of the untracked file search when the
    `core.untrackedCache` and `core.fsmonitor` features are enabled by
    caching results from previous `git status` invocations.

    Therefore, update the `git status` man page to explain the various
    configuration options, and update the advice to provide more
    detail about the current configuration and to refer to the updated
    documentation.

Second, we usually don't want to waste a test script number (such as
"t7065") if we can avoid it, especially for so few tests and such
minor functionality. So, if there is an existing test script in which
these new tests might fit, it's better to add them to that script
instead, usually at the end of the script. (I haven't checked, but
maybe you can find an existing script which would be a good fit; if
not, then placing them in a new standalone script, as the patch is
already doing, may be okay.)

  reply	other threads:[~2022-11-21 16:18 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
2022-11-21 15:54             ` Rudy Rigot
2022-11-21 16:17               ` Eric Sunshine [this message]
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+cQgu=i6pZTzoNYGZ_6X=DGdmwa=dPhSQVqD+eLCZCGJSg@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).