Re: [PATCH v1 1/3] read-cache: add post-indexchanged hook

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Feb 8, 2019, 23:53 UTC
Message-ID: <20190208235317.GI11927@genre.crustytoothpaste.net>
In-Reply-To: <20190208195115.12156-2-peartben@gmail.com>

On Fri, Feb 08, 2019 at 02:51:13PM -0500, Ben Peart wrote:

Show 9 quoted lines

> From: Ben Peart <benpeart@microsoft.com>
> 
> Add a post-indexchanged hook that is invoked after the index is written in
> do_write_locked_index().
> 
> This hook is meant primarily for notification, and cannot affect
> the outcome of git commands that trigger the index write.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>

First, I think the tests should be merged into this commit. That's what we typically do.

I'm also going to bikeshed slightly and suggest "post-index-changed", since we normally use dashes between words in our hook names.

Show 12 quoted lines

> diff --git a/cache.h b/cache.h
> index 27fe635f62..46eb862d3e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -338,7 +338,9 @@ struct index_state {
>  	struct cache_time timestamp;
>  	unsigned name_hash_initialized : 1,
>  		 initialized : 1,
> -		 drop_cache_tree : 1;
> +		 drop_cache_tree : 1,
> +		 updated_workdir : 1,
> +		 updated_skipworktree : 1;

How important is it that we expose whether the skip-worktree bit is changed? I can understand if we expose the workdir is updated, since that's a thing a general user of this hook is likely to be interested in. However, I'm not sure that for a general-purpose hook, the skip-worktree bit is interesting.

Show 25 quoted lines

> diff --git a/read-cache.c b/read-cache.c
> index 0e0c93edc9..0fcfa8a075 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -17,6 +17,7 @@
>  #include "commit.h"
>  #include "blob.h"
>  #include "resolve-undo.h"
> +#include "run-command.h"
>  #include "strbuf.h"
>  #include "varint.h"
>  #include "split-index.h"
> @@ -2999,8 +3000,17 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
>  	if (ret)
>  		return ret;
>  	if (flags & COMMIT_LOCK)
> -		return commit_locked_index(lock);
> -	return close_lock_file_gently(lock);
> +		ret = commit_locked_index(lock);
> +	else
> +		ret = close_lock_file_gently(lock);
> +
> +	run_hook_le(NULL, "post-indexchanged",
> +			istate->updated_workdir ? "1" : "0",
> +			istate->updated_skipworktree ? "1" : "0", NULL);

I have, in general, some concerns about this API. First, I think we need to consider that if we're going to expose various bits of information, we might in the future want to expose more such bits. If so, adding integer parameters is not likely to be a good way to do this. It's hard to remember and if a binary is used as the hook, it may not always handle additional arguments gracefully like shell scripts tend to.

If we're not going to expose the skip-worktree bit, then I suppose one argument is fine. Otherwise, it might be better to expose key-value pairs on stdin instead, or something like that.

Finally, I have questions about performance. What's the overhead of determining whether the hook exists in this code path when there isn't one? Since the index is frequently used, and can be written out as an optimization by some commands, it would be nice to keep overhead low if the hook isn't present.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204