Subject: Re: [PATCH v3 02/34] fsmonitor--daemon: man page

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 13 Jul 2021 19:46:07 +0200

On Mon, Jul 12 2021, Jeff Hostetler wrote:

> On 7/1/21 6:29 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:
>> 
>>> From: Jeff Hostetler <[email-masked]>
>>>
>>> Create a manual page describing the `git fsmonitor--daemon` feature.
>>>
>>> Signed-off-by: Jeff Hostetler <[email-masked]>
>>> ---
>>>   Documentation/git-fsmonitor--daemon.txt | 75 +++++++++++++++++++++++++
>>>   1 file changed, 75 insertions(+)
>>>   create mode 100644 Documentation/git-fsmonitor--daemon.txt
>>>
>>> diff --git a/Documentation/git-fsmonitor--daemon.txt b/Documentation/git-fsmonitor--daemon.txt
>>> new file mode 100644
>>> index 00000000000..154e7684daa
>>> --- /dev/null
>>> +++ b/Documentation/git-fsmonitor--daemon.txt
>>> @@ -0,0 +1,75 @@
>>> +git-fsmonitor--daemon(1)
>>> +========================
>>> +
>>> +NAME
>>> +----
>>> +git-fsmonitor--daemon - A Built-in File System Monitor
>>> +
>>> +SYNOPSIS
>>> +--------
>>> +[verse]
>>> +'git fsmonitor--daemon' start
>>> +'git fsmonitor--daemon' run
>>> +'git fsmonitor--daemon' stop
>>> +'git fsmonitor--daemon' status
>>> +
>>> +DESCRIPTION
>>> +-----------
>>> +
>>> +A daemon to watch the working directory for file and directory
>>> +changes using platform-specific file system notification facilities.
>>> +
>>> +This daemon communicates directly with commands like `git status`
>>> +using the link:technical/api-simple-ipc.html[simple IPC] interface
>>> +instead of the slower linkgit:githooks[5] interface.
>>> +
>>> +This daemon is built into Git so that no third-party tools are
>>> +required.
>>> +
>>> +OPTIONS
>>> +-------
>>> +
>>> +start::
>>> +	Starts a daemon in the background.
>>> +
>>> +run::
>>> +	Runs a daemon in the foreground.
>>> +
>>> +stop::
>>> +	Stops the daemon running in the current working
>>> +	directory, if present.
>>> +
>>> +status::
>>> +	Exits with zero status if a daemon is watching the
>>> +	current working directory.
>>> +
>>> +REMARKS
>>> +-------
>>> +
>>> +This daemon is a long running process used to watch a single working
>>> +directory and maintain a list of the recently changed files and
>>> +directories.  Performance of commands such as `git status` can be
>>> +increased if they just ask for a summary of changes to the working
>>> +directory and can avoid scanning the disk.
>>> +
>>> +When `core.useBuiltinFSMonitor` is set to `true` (see
>>> +linkgit:git-config[1]) commands, such as `git status`, will ask the
>>> +daemon for changes and automatically start it (if necessary).
>>> +
>>> +For more information see the "File System Monitor" section in
>>> +linkgit:git-update-index[1].
>>> +
>>> +CAVEATS
>>> +-------
>>> +
>>> +The fsmonitor daemon does not currently know about submodules and does
>>> +not know to filter out file system events that happen within a
>>> +submodule.  If fsmonitor daemon is watching a super repo and a file is
>>> +modified within the working directory of a submodule, it will report
>>> +the change (as happening against the super repo).  However, the client
>>> +will properly ignore these extra events, so performance may be affected
>>> +but it will not cause an incorrect result.
>>> +
>>> +GIT
>>> +---
>>> +Part of the linkgit:git[1] suite
>> Later in the series we incrementally add features to the daemon, so
>> this
>> is describing a state that doesn't exist yet at this point.
>> I think it would be better to start with a stup here and add
>> documentation as we add features, e.g. the patch tha adds "start" should
>> add that to the synopsis + options etc.
>> See the outstanding ab/config-based-hooks-base for a small example
>> of
>> that.
>> 
>
> I like to lead the series with the documentation that summarizes the
> purpose of the entire feature or patch series.  This gives the reviewer
> the context for the complete series that follows.  In the past, we've
> had discussions on the list about how hard it is to review a series when
> the foo.c comes (alphabetically) before foo.h in the patch and all
> the documentation is attached to the prototypes in the .h file so the
> reviewer needs to bounce around in the patch or series to read the
> intent and then go back to the beginning to read the code.  In that
> spirit, I think that having the complete man page come first provides
> necessary context and is helpful.
>
> The argument that the man-page should grow as the feature grows
> presumes that there is a meaningful cut-point mid-series where you
> would adopt the first portion and delay the second to a later release
> or something.  That division would not be useful/usable.

Isn't there such a meaningful cut-off point?

In 08/34[1] you add a skeleton of the daemon, so then the NAME/SYNOPSIS
(empty)/DESCRIPTION/REMARKS/GIT sections could be added, in 09/34[3] you
add stop/status commands, so then the SYNOPSIS/OPTIONS could be
updated/created with those, same for the addition of run/start in
13/34[5] and 14/35[6] respectively.

> And it just clutters up later commits in the series with man-page
> deltas.
>
> So I'd like to keep it as it unless there are further objections.

I'll leave it to you to do with the feedback as you choose,

I suggested this because I think it's much easier to read patches that
are larger because they incrementally update docs or tests along with
code, than smaller ones that are e.g. "add docs" followed by
incrementally modifying the code.

That's because you can consider those atomically. If earlier doc changes
refer to later code changes you're left jumping back & forth and
wondering if the code you're reading that doesn't match the docs yet is
a bug, or if it's solved in some later change you're now needing to
mentally keep track of.

Which is not some theoretical concern b.t.w., but exactly what I found
myself doing when reading this series, hence the suggestion.

1. https://lore.kernel.org/git/f88d[email-masked]/
2. https://lore.kernel.org/git/[email-masked]/
3. https://lore.kernel.org/git/02e2[email-masked]/
4. https://lore.kernel.org/git/5d66[email-masked]/
5. https://lore.kernel.org/git/5d66[email-masked]/
6. https://lore.kernel.org/git/9fe9[email-masked]/