| Message ID | 5db2c0390a657d5790cf24404201505c4ec3a829.1625150864.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Builtin FSMonitor Feature | expand |
On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@microsoft.com> > > Create a manual page describing the `git fsmonitor--daemon` feature. > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > --- > 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.
Hi Ævar, On Fri, 2 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: > > > From: Jeff Hostetler <jeffhost@microsoft.com> > > > > Create a manual page describing the `git fsmonitor--daemon` feature. > > > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > > --- > > 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. Incidentally, I had structured the patch series that way until Jeff took over from me last year. And it was definitely not more reviewable because it was not clear from the get-go what I intended this command to do. Therefore, I object to your suggestion. Ciao, Johannes P.S.: I will not address your other reviews in this patch series, mostly because I am technically off from work this week.
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 <jeffhost@microsoft.com> >> >> Create a manual page describing the `git fsmonitor--daemon` feature. >> >> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> >> --- >> 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. 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. Jeff
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 <jeffhost@microsoft.com> >>> >>> Create a manual page describing the `git fsmonitor--daemon` feature. >>> >>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> >>> --- >>> 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/f88db92d4259d1c29827e97e957daf6eda39c551.1625150864.git.gitgitgadget@gmail.com/ 2. https://lore.kernel.org/git/877di9d5uz.fsf@evledraar.gmail.com/ 3. https://lore.kernel.org/git/02e21384ef0ca4909e0bda2c78fa63c06be22a50.1625150864.git.gitgitgadget@gmail.com/ 4. https://lore.kernel.org/git/5d6646df93a17659af66f136295444d1bd834090.1625150864.git.gitgitgadget@gmail.com/ 5. https://lore.kernel.org/git/5d6646df93a17659af66f136295444d1bd834090.1625150864.git.gitgitgadget@gmail.com/ 6. https://lore.kernel.org/git/9fe902aad87f1192705fb69ea212a2d066d0286d.1625150864.git.gitgitgadget@gmail.com/
Hi Ævar, On Tue, 13 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > [snip Ævar's suggestion to populate the manual page incrementally, > interspersed with the commits that finalize implementing the respective > functionality] > > 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. My experience is the exact opposite of yours: shorter patches are easier to read. > That's because you can consider those atomically. No, in a patch series you cannot consider any patch completely atomically. Just like you don't consider any paragraph in any well-written book out of context. > 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. You only keep jumping back and forth when reviewing _patches_. We try to do code review on this mailing list, which means that you have the code locally and review it in the correct context. When you do that, a design document is quite helpful. And the proposed manual page serves as such a design document. > Which is not some theoretical concern b.t.w., but exactly what I found > myself doing when reading this series, hence the suggestion. Part of the problem here seems to be that this patch series saw many reviewer suggestions that forced it to increase in length. That was probably not very helpful, after all. The first iteration had 23 patches. Reviews forced it to grow to 28 patches in the second iteration. And that was still not enough, therefore the third iteration consisted of 34 patches. And if you had had your way, requiring Jeff to include a Linux backend in the same patch series, it would have to increase in size again, and not just by a little. This all sounds like we're truly falling into the trap of ignoring the rule that the perfect is the enemy of the good. I really would like to come back to a focused review that truly improves the patches at hand, and avoids conflating the review of the actual patch series with matters of personal taste (which is a recipe for disagreement). After all, we are interested in getting this feature out to users who need to work with very large worktrees, right? At least that's my goal here. Ciao, Johannes
Johannes Schindelin wrote: > On Tue, 13 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > > > [snip Ævar's suggestion to populate the manual page incrementally, > > interspersed with the commits that finalize implementing the respective > > functionality] > > > > 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. > > My experience is the exact opposite of yours: shorter patches are easier > to read. It depends. > > That's because you can consider those atomically. > > No, in a patch series you cannot consider any patch completely atomically. > Just like you don't consider any paragraph in any well-written book out of > context. But you do not put every sentence in a paragraph. Sometimes a paragraph can contain a single sentence, or a single word even. But other times to properly read what is being tried to say you need a pretty big paragraph. > This all sounds like we're truly falling into the trap of ignoring the > rule that the perfect is the enemy of the good. Have you established that this is good enough?
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