Re: [PATCH v1 2/5] Teach git to optionally utilize a file system monitor to speed up detecting new or changed files.
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- May 16, 2017, 21:41 UTC
- Message-ID
- <473c4b47-06a7-cb55-6d67-e335fa5b5a5b@google.com>
- In-Reply-To
- <20170515191347.1892-3-benpeart@microsoft.com>
I'm not very familiar with this part of the code - here is a partial review.
Firstly, if someone invokes update-index, I wonder if it's better just to do a full refresh (e.g. by deleting the last_update time from the index).
Also, the change to unpack-trees.c doesn't match my mental model. I notice that it is in a function related to sparse checkout, but if the working tree changes for whatever reason, it seems simpler to just let the hook do its thing. As far as I can tell, it is fine to have files overzealously marked as FSMONITOR_DIRTY.
On 05/15/2017 12:13 PM, Ben Peart wrote:
Show 26 quoted lines
> diff --git a/cache.h b/cache.h
> index 40ec032a2d..64aa6e57cd 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -201,6 +201,7 @@ struct cache_entry {
> #define CE_ADDED (1 << 19)
>
> #define CE_HASHED (1 << 20)
> +#define CE_FSMONITOR_DIRTY (1 << 21)
> #define CE_WT_REMOVE (1 << 22) /* remove in work directory */
> #define CE_CONFLICTED (1 << 23)
>
> @@ -324,6 +325,7 @@ static inline unsigned int canon_mode(unsigned int mode)
> #define CACHE_TREE_CHANGED (1 << 5)
> #define SPLIT_INDEX_ORDERED (1 << 6)
> #define UNTRACKED_CHANGED (1 << 7)
> +#define FSMONITOR_CHANGED (1 << 8)
>
> struct split_index;
> struct untracked_cache;
> @@ -342,6 +344,8 @@ struct index_state {
> struct hashmap dir_hash;
> unsigned char sha1[20];
> struct untracked_cache *untracked;
> + time_t last_update;
> + struct ewah_bitmap *bitmap;Here a bitmap is introduced, presumably corresponding to the entries in "struct cache_entry **cache", but there is also a CE_FSMONITOR_DIRTY that can be set in each "struct cache_entry". This seems redundant and probably at least worth explaining in a comment.
Show 20 quoted lines
> +/*
> + * Call the query-fsmonitor hook passing the time of the last saved results.
> + */
> +static int query_fsmonitor(time_t last_update, struct strbuf *buffer)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + char date[64];
> + const char *argv[3];
> +
> + if (!(argv[0] = find_hook("query-fsmonitor")))
> + return -1;
> +
> + snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
> + argv[1] = date;
> + argv[2] = NULL;
> + cp.argv = argv;
> + cp.out = -1;
> +
> + return capture_command(&cp, buffer, 1024);
> +}Output argument could probably be named better.
Also, would the output of this command be very large? If yes, it might be better to process it little by little instead of buffering the whole thing first.
> +void write_fsmonitor_extension(struct strbuf *sb, struct index_state* istate);
Space before * (in the .h and .c files).