Re: [PATCH v5 0/7] Fast git status via a file system watcher

[Ben Peart <peartben@gmail.com>]

On 6/28/2017 1:11 AM, Christian Couder wrote:
> On Sat, Jun 10, 2017 at 3:40 PM, Ben Peart <peartben@gmail.com> wrote:
>> Changes from V4 include:
> ...
> 
> I took a look at this patch series except the last patch ([PATCH v5
> 7/7] fsmonitor: add a performance test) as Junio reviewed it already,
> and had only a few comments on patches 3/7 and 4/7.
> 
> I am still not convinced by the discussions following v2
> (http://public-inbox.org/git/20170518201333.13088-1-benpeart@microsoft.com/)
> about using a hook instead of for example a "core.fsmonitorcommand".
> 
> I think using a hook is not necessary and might not be a good match
> for later optimizations. For example people might want to use a
> library or some OS specific system calls to do what the hook does.
> 
> AEvar previously reported some not so great performance numbers on
> some big Booking.com boxes with a big monorepo and it seems that using
> taskset for example to make sure that the hook is run on the same CPU
> improves these numbers significantly. So avoiding to run a separate
> process can be important in some cases.
> 

Using a hook is the only pattern I've seen in git that provides a way to 
enable OS specific calls.  I used a hook so that different file 
monitoring services could be plugged in depending on the OS or tools 
available.  The Watchman integration script was mostly intended as a 
sample that could be used where Watchman is available and works well.

I had not heard about the taskset issues you mention above.  If there is 
something else that can be done to make this work better, please let me 
know the details.

If I'm understanding you correctly, you are suggesting that someone 
should be able to configure a setting (core.fsmonitorcommand) that gives 
a custom command line that would be run instead of running the 
query-fsmonitor hook.

I'm not entirely sure how that should work.  There are command line 
options that need to be passed (currently the interface version as well 
as the current clock in nanoseconds).  How would those passed when using 
the custom command?

Is it OK to just append them to the given command line?  Does there need 
to be some substitution token to indicate where they should be inserted 
(ie "mycustomcommand --custom --options %version% --more-options 
%timestamp%").  Are there any other tokens that should be supported (ie 
PID or processor mask?).

Is there a design pattern already used somewhere in git that I can 
follow or is this all blazing a new trail?

Thanks for continuing to look into this. Feedback is good!