Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 01/23] fsmonitor--daemon: man page and documentation
Date: Mon, 26 Apr 2021 10:13:15 -0400
Message-ID: <b868522c-f07f-c431-aa2c-14af84015ced@gmail.com> (raw)
In-Reply-To: <074273330f8d6c656dfec7c8778fad20314c6ad1.1617291666.git.gitgitgadget@gmail.com>

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Create a manual page describing the `git fsmonitor--daemon` feature.
> 
> Update references to `core.fsmonitor`, `core.fsmonitorHookVersion` and
> pointers to `watchman` to mention the built-in FSMonitor.

Make sense to add clarity here, since there will be new ways
to interact with a fileystem monitor.
>  core.fsmonitorHookVersion::
> -	Sets the version of hook that is to be used when calling fsmonitor.
> -	There are currently versions 1 and 2. When this is not set,
> -	version 2 will be tried first and if it fails then version 1
> -	will be tried. Version 1 uses a timestamp as input to determine
> -	which files have changes since that time but some monitors
> -	like watchman have race conditions when used with a timestamp.
> -	Version 2 uses an opaque string so that the monitor can return
> -	something that can be used to determine what files have changed
> -	without race conditions.
> +	Sets the version of hook that is to be used when calling the
> +	FSMonitor hook (as configured via `core.fsmonitor`).
> ++
> +There are currently versions 1 and 2. When this is not set,
> +version 2 will be tried first and if it fails then version 1
> +will be tried. Version 1 uses a timestamp as input to determine
> +which files have changes since that time but some monitors
> +like watchman have race conditions when used with a timestamp.
> +Version 2 uses an opaque string so that the monitor can return
> +something that can be used to determine what files have changed
> +without race conditions.

This initially seemed like a big edit, but you just split the single
paragraph into multiple, with a better leading sentence and a final
statement about the built-in FSMonitor. Good.
> ++
> +Note: FSMonitor hooks (and this config setting) are ignored if the
> +built-in FSMonitor is enabled (see `core.useBuiltinFSMonitor`).
> +
> +core.useBuiltinFSMonitor::
> +	If set to true, enable the built-in filesystem event watcher (for
> +	technical details, see linkgit:git-fsmonitor--daemon[1]).
> ++
> +Like external (hook-based) FSMonitors, the built-in FSMonitor can speed up
> +Git commands that need to refresh the Git index (e.g. `git status`) in a
> +worktree with many files. The built-in FSMonitor facility eliminates the
> +need to install and maintain an external third-party monitoring tool.
> ++
> +The built-in FSMonitor is currently available only on a limited set of
> +supported platforms.

Is there a way for users to know this set of platforms? Can they run
a command to find out? Will 'git fsmonitor--daemon --start' send a
helpful message to assist here? Or, could there be a 'git
fsmonitor--daemon --test' command?

> +Note: if this config setting is set to `true`, any FSMonitor hook
> +configured via `core.fsmonitor` (and possibly `core.fsmonitorHookVersion`)
> +is ignored.
...
> +git-fsmonitor--daemon(1)
> +========================
> +
> +NAME
> +----
> +git-fsmonitor--daemon - Builtin file system monitor daemon
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git fsmonitor--daemon' --start
> +'git fsmonitor--daemon' --run
> +'git fsmonitor--daemon' --stop
> +'git fsmonitor--daemon' --is-running
> +'git fsmonitor--daemon' --is-supported
> +'git fsmonitor--daemon' --query <token>
> +'git fsmonitor--daemon' --query-index
> +'git fsmonitor--daemon' --flush

These arguments with the "--" prefix make it seem like they are
options that could be grouped together, but you really want these
to be verbs within the daemon. What do you think about removing
the "--" prefixes?

> +
> +DESCRIPTION
> +-----------
> +
> +Monitors files and directories in the working directory for changes using
> +platform-specific file system notification facilities.
> +
> +It 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.
> +
> +OPTIONS
> +-------

I typically view "OPTIONS" as arguments that can be grouped together,
but you are describing things more like verbs or subcommands. The
most recent example I know about is 'git maintenance <subcommand>',
documented at [1].

[1] https://git-scm.com/docs/git-maintenance#_subcommands

> +
> +--start::
> +	Starts the fsmonitor daemon in the background.
> +
> +--run::
> +	Runs the fsmonitor daemon in the foreground.
> +
> +--stop::
> +	Stops the fsmonitor daemon running for the current working
> +	directory, if present.

I'm noticing "fsmonitor" in lowercase throughout this document. Is
that the intended case for user-facing documentation? I've been 
seeing "FS Monitor", "filesystem monitor", or even "File System
Monitor" in other places.

> +--is-running::
> +	Exits with zero status if the fsmonitor daemon is watching the
> +	current working directory.

Another potential name for this verb is "status".

> +--is-supported::
> +	Exits with zero status if the fsmonitor daemon feature is supported
> +	on this platform.

Ah, here is an indicator of whether the platform is supported. Please
include details for this command in the earlier documentation. I'll
check later to see if a message is also sent over 'stderr', which
would be helpful. Documenting the exit status is good for third-party
tools that might use this.

> +--query <token>::
> +	Connects to the fsmonitor daemon (starting it if necessary) and
> +	requests the list of changed files and directories since the
> +	given token.
> +	This is intended for testing purposes.
> +
> +--query-index::
> +	Read the current `<token>` from the File System Monitor index
> +	extension (if present) and use it to query the fsmonitor daemon.
> +	This is intended for testing purposes.

These two could be grouped as "query [--token=X|--index]", especially
because they are for testing purposes.

> +
> +--flush::
> +	Force the fsmonitor daemon to flush its in-memory cache and
> +	re-sync with the file system.
> +	This is intended for testing purposes.

Do you see benefits to these being available in the CLI? Could these
be better served as a test helper?

> +REMARKS
> +-------
> +The fsmonitor daemon is a long running process that will watch a single
> +working directory.  Commands, such as `git status`, should automatically
> +start it (if necessary) when `core.useBuiltinFSMonitor` is set to `true`
> +(see linkgit:git-config[1]).
> +
> +Configure the built-in FSMonitor via `core.useBuiltinFSMonitor` in each
> +working directory separately, or globally via `git config --global
> +core.useBuiltinFSMonitor true`.
> +
> +Tokens are opaque strings.  They are used by the fsmonitor daemon to
> +mark a point in time and the associated internal state.  Callers should
> +make no assumptions about the content of the token.  In particular,
> +the should not assume that it is a timestamp.
> +
> +Query commands send a request-token to the daemon and it responds with
> +a summary of the changes that have occurred since that token was
> +created.  The daemon also returns a response-token that the client can
> +use in a future query.
> +
> +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
> +should properly ignore these extra events, so performance may be affected
> +but it should not cause an incorrect result.

There are several uses of the word "should" where I think "will" is a
more appropriate word. That is, unless we do not actually have confidence
in this behavior.

> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -498,7 +498,9 @@ FILE SYSTEM MONITOR
>  This feature is intended to speed up git operations for repos that have
>  large working directories.
>  
> -It enables git to work together with a file system monitor (see the
> +It enables git to work together with a file system monitor (see
> +linkgit:git-fsmonitor--daemon[1]
> +and the
>  "fsmonitor-watchman" section of linkgit:githooks[5]) that can
>  inform it as to what files have been modified. This enables git to avoid
>  having to lstat() every file to find modified files.
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index b51959ff9418..b7d5e926f7b0 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -593,7 +593,8 @@ fsmonitor-watchman
>  
>  This hook is invoked when the configuration option `core.fsmonitor` is
>  set to `.git/hooks/fsmonitor-watchman` or `.git/hooks/fsmonitor-watchmanv2`
> -depending on the version of the hook to use.
> +depending on the version of the hook to use, unless overridden via
> +`core.useBuiltinFSMonitor` (see linkgit:git-config[1]).
>  
>  Version 1 takes two arguments, a version (1) and the time in elapsed
>  nanoseconds since midnight, January 1, 1970.

These are good connections to make.

Since the documentation for the fsmonitor--daemon is so deep, this
patch might be served well to split into two: one that just documents
the daemon, and another that updates existing documentation to point
to the new file.

This does provide a good basis for me to investigate during the rest
of the review.

Thanks,
-Stolee

  reply index

Thread overview: 118+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-01 15:40 [PATCH 00/23] [RFC] Builtin FSMonitor Feature Jeff Hostetler via GitGitGadget
2021-04-01 15:40 ` [PATCH 01/23] fsmonitor--daemon: man page and documentation Jeff Hostetler via GitGitGadget
2021-04-26 14:13   ` Derrick Stolee [this message]
2021-04-28 13:54     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 02/23] fsmonitor-ipc: create client routines for git-fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-04-26 14:31   ` Derrick Stolee
2021-04-26 20:20     ` Eric Sunshine
2021-04-26 21:02       ` Derrick Stolee
2021-04-28 19:26     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 03/23] config: FSMonitor is repository-specific Johannes Schindelin via GitGitGadget
2021-04-01 15:40 ` [PATCH 04/23] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC Johannes Schindelin via GitGitGadget
2021-04-26 14:56   ` Derrick Stolee
2021-04-27  9:20     ` Ævar Arnfjörð Bjarmason
2021-04-27 12:42       ` Derrick Stolee
2021-04-28  7:59         ` Ævar Arnfjörð Bjarmason
2021-04-28 16:26           ` [PATCH] repo-settings.c: simplify the setup Ævar Arnfjörð Bjarmason
2021-04-28 19:09             ` Nesting topics within other threads (was: [PATCH] repo-settings.c: simplify the setup) Derrick Stolee
2021-04-28 23:01               ` Ævar Arnfjörð Bjarmason
2021-05-05 16:12                 ` Johannes Schindelin
2021-04-29  5:12               ` Nesting topics within other threads Junio C Hamano
2021-04-29 12:14                 ` Ævar Arnfjörð Bjarmason
2021-04-29 20:14                   ` Jeff King
2021-04-30  0:07                   ` Junio C Hamano
2021-04-30 14:23     ` [PATCH 04/23] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC Jeff Hostetler
2021-04-01 15:40 ` [PATCH 05/23] fsmonitor--daemon: add a built-in fsmonitor daemon Jeff Hostetler via GitGitGadget
2021-04-26 15:08   ` Derrick Stolee
2021-04-26 15:45     ` Derrick Stolee
2021-04-30 14:31       ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 06/23] fsmonitor--daemon: implement client command options Jeff Hostetler via GitGitGadget
2021-04-26 15:12   ` Derrick Stolee
2021-04-30 14:33     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 07/23] fsmonitor-fs-listen-win32: stub in backend for Windows Jeff Hostetler via GitGitGadget
2021-04-26 15:23   ` Derrick Stolee
2021-04-01 15:40 ` [PATCH 08/23] fsmonitor-fs-listen-macos: stub in backend for MacOS Jeff Hostetler via GitGitGadget
2021-04-01 15:40 ` [PATCH 09/23] fsmonitor--daemon: implement daemon command options Jeff Hostetler via GitGitGadget
2021-04-26 15:47   ` Derrick Stolee
2021-04-26 16:12     ` Derrick Stolee
2021-04-30 15:18       ` Jeff Hostetler
2021-04-30 15:59     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 10/23] fsmonitor--daemon: add pathname classification Jeff Hostetler via GitGitGadget
2021-04-26 19:17   ` Derrick Stolee
2021-04-26 20:11     ` Eric Sunshine
2021-04-26 20:24       ` Derrick Stolee
2021-04-01 15:40 ` [PATCH 11/23] fsmonitor--daemon: define token-ids Jeff Hostetler via GitGitGadget
2021-04-26 19:49   ` Derrick Stolee
2021-04-26 20:01     ` Eric Sunshine
2021-04-26 20:03       ` Derrick Stolee
2021-04-30 16:17     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 12/23] fsmonitor--daemon: create token-based changed path cache Jeff Hostetler via GitGitGadget
2021-04-26 20:22   ` Derrick Stolee
2021-04-30 17:36     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 13/23] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows Jeff Hostetler via GitGitGadget
2021-04-27 17:22   ` Derrick Stolee
2021-04-27 17:41     ` Eric Sunshine
2021-04-30 19:32     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 14/23] fsmonitor-fs-listen-macos: add macos header files for FSEvent Jeff Hostetler via GitGitGadget
2021-04-27 18:13   ` Derrick Stolee
2021-04-01 15:40 ` [PATCH 15/23] fsmonitor-fs-listen-macos: implement FSEvent listener on MacOS Jeff Hostetler via GitGitGadget
2021-04-27 18:35   ` Derrick Stolee
2021-04-30 20:05     ` Jeff Hostetler
2021-04-01 15:40 ` [PATCH 16/23] fsmonitor--daemon: implement handle_client callback Jeff Hostetler via GitGitGadget
2021-04-26 21:01   ` Derrick Stolee
2021-05-03 15:04     ` Jeff Hostetler
2021-05-13 18:52   ` Derrick Stolee
2021-04-01 15:40 ` [PATCH 17/23] fsmonitor--daemon: periodically truncate list of modified files Jeff Hostetler via GitGitGadget
2021-04-27 13:24   ` Derrick Stolee
2021-04-01 15:41 ` [PATCH 18/23] fsmonitor--daemon:: introduce client delay for testing Jeff Hostetler via GitGitGadget
2021-04-27 13:36   ` Derrick Stolee
2021-04-01 15:41 ` [PATCH 19/23] fsmonitor--daemon: use a cookie file to sync with file system Jeff Hostetler via GitGitGadget
2021-04-27 14:23   ` Derrick Stolee
2021-05-03 21:59     ` Jeff Hostetler
2021-04-01 15:41 ` [PATCH 20/23] fsmonitor: force update index when fsmonitor token advances Jeff Hostetler via GitGitGadget
2021-04-27 14:52   ` Derrick Stolee
2021-04-01 15:41 ` [PATCH 21/23] t7527: create test for fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-04-27 15:41   ` Derrick Stolee
2021-04-01 15:41 ` [PATCH 22/23] p7519: add fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-04-27 15:45   ` Derrick Stolee
2021-04-01 15:41 ` [PATCH 23/23] t7527: test status with untracked-cache and fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-04-27 15:51   ` Derrick Stolee
2021-04-16 22:44 ` [PATCH 00/23] [RFC] Builtin FSMonitor Feature Junio C Hamano
2021-04-20 15:27   ` Johannes Schindelin
2021-04-20 19:13     ` Jeff Hostetler
2021-04-21 13:17     ` Derrick Stolee
2021-04-27 18:49 ` FS Monitor Windows Performance (was [PATCH 00/23] [RFC] Builtin FSMonitor Feature) Derrick Stolee
2021-04-27 19:31 ` FS Monitor macOS " Derrick Stolee
2021-05-22 13:56 ` [PATCH v2 00/28] Builtin FSMonitor Feature Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 01/28] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 02/28] fsmonitor--daemon: man page Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 03/28] fsmonitor--daemon: update fsmonitor documentation Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 04/28] fsmonitor-ipc: create client routines for git-fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-06-02 11:24     ` Johannes Schindelin
2021-05-22 13:56   ` [PATCH v2 05/28] help: include fsmonitor--daemon feature flag in version info Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 06/28] config: FSMonitor is repository-specific Johannes Schindelin via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 07/28] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC Johannes Schindelin via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 08/28] fsmonitor--daemon: add a built-in fsmonitor daemon Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 09/28] fsmonitor--daemon: implement client command options Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 10/28] t/helper/fsmonitor-client: create IPC client to talk to FSMonitor Daemon Jeff Hostetler via GitGitGadget
2021-06-11  6:32     ` Junio C Hamano
2021-05-22 13:56   ` [PATCH v2 11/28] fsmonitor-fs-listen-win32: stub in backend for Windows Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 12/28] fsmonitor-fs-listen-macos: stub in backend for MacOS Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 13/28] fsmonitor--daemon: implement daemon command options Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 14/28] fsmonitor--daemon: add pathname classification Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 15/28] fsmonitor--daemon: define token-ids Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 16/28] fsmonitor--daemon: create token-based changed path cache Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 17/28] fsmonitor-fs-listen-win32: implement FSMonitor backend on Windows Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 18/28] fsmonitor-fs-listen-macos: add macos header files for FSEvent Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 19/28] fsmonitor-fs-listen-macos: implement FSEvent listener on MacOS Jeff Hostetler via GitGitGadget
2021-05-22 13:56   ` [PATCH v2 20/28] fsmonitor--daemon: implement handle_client callback Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 21/28] fsmonitor--daemon: periodically truncate list of modified files Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 22/28] fsmonitor--daemon: use a cookie file to sync with file system Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 23/28] fsmonitor: enhance existing comments Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 24/28] fsmonitor: force update index after large responses Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 25/28] t7527: create test for fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 26/28] p7519: add fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 27/28] t7527: test status with untracked-cache and fsmonitor--daemon Jeff Hostetler via GitGitGadget
2021-05-22 13:57   ` [PATCH v2 28/28] t/perf: avoid copying builtin fsmonitor files into test repo Jeff Hostetler via GitGitGadget
2021-05-27  2:06   ` [PATCH v2 00/28] Builtin FSMonitor Feature Junio C Hamano
2021-06-02 11:28     ` Johannes Schindelin

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

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

  git send-email \
    --in-reply-to=b868522c-f07f-c431-aa2c-14af84015ced@gmail.com \
    --to=stolee@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.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

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git