Re: [PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Junio C Hamano <gitster@pobox.com>]

"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nipunn Koorapati <nipunn@dropbox.com>
>
> Validate that fsmonitor is valid to futureproof against bugs where
> check_removed might be called from places that haven't refreshed.

Isn't this the other way around, wrt to the previous step?

At least, "pass around istate throughout the callchain in the
diff-lib.c file" change should stand alone and come much earlier in
the series (perhaps as step #1).  Then "call refresh_fsmonitor from
run_diff_index() and make sure in check_removed() that fsmonitor
does not have bogus VALID bit" assertion should come on top, as a
single step, I would think.

> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
>  diff-lib.c  | 18 +++++++++++-------
>  fsmonitor.h | 11 +++++++++++
>  2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index 3fb538ad18e9..e5a58c9259cf 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -28,8 +28,9 @@
>   * exists for ce that is a submodule -- it is a submodule that is not
>   * checked out).  Return negative for an error.
>   */
> -static int check_removed(const struct cache_entry *ce, struct stat *st)
> +static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
>  {
> +	assert(is_fsmonitor_refreshed(istate));
>  	if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
>  		if (!is_missing_file_error(errno))
>  			return -1;
> @@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			memset(&(dpath->parent[0]), 0,
>  			       sizeof(struct combine_diff_parent)*5);
>  
> -			changed = check_removed(ce, &st);
> +			changed = check_removed(istate, ce, &st);
>  			if (!changed)
>  				wt_mode = ce_mode_from_stat(ce, st.st_mode);
>  			else {
> @@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  		} else {
>  			struct stat st;
>  
> -			changed = check_removed(ce, &st);
> +			changed = check_removed(istate, ce, &st);
>  			if (changed) {
>  				if (changed < 0) {
>  					perror(ce->name);
> @@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs,
>  		       oid, oid_valid, ce->name, dirty_submodule);
>  }
>  
> -static int get_stat_data(const struct cache_entry *ce,
> +static int get_stat_data(const struct index_state *istate,
> +			 const struct cache_entry *ce,
>  			 const struct object_id **oidp,
>  			 unsigned int *modep,
>  			 int cached, int match_missing,
> @@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce,
>  	if (!cached && !ce_uptodate(ce)) {
>  		int changed;
>  		struct stat st;
> -		changed = check_removed(ce, &st);
> +		changed = check_removed(istate, ce, &st);
>  		if (changed < 0)
>  			return -1;
>  		else if (changed) {
> @@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs,
>  	const struct object_id *oid;
>  	unsigned int mode;
>  	unsigned dirty_submodule = 0;
> +	struct index_state *istate = revs->diffopt.repo->index;
>  
>  	/*
>  	 * New file in the index: it might actually be different in
>  	 * the working tree.
>  	 */
> -	if (get_stat_data(new_file, &oid, &mode, cached, match_missing,
> +	if (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing,
>  	    &dirty_submodule, &revs->diffopt) < 0)
>  		return;
>  
> @@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs,
>  	unsigned int mode, oldmode;
>  	const struct object_id *oid;
>  	unsigned dirty_submodule = 0;
> +	struct index_state *istate = revs->diffopt.repo->index;
>  
> -	if (get_stat_data(new_entry, &oid, &mode, cached, match_missing,
> +	if (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,
>  			  &dirty_submodule, &revs->diffopt) < 0) {
>  		if (report_missing)
>  			diff_index_show_file(revs, "-", old_entry,
> diff --git a/fsmonitor.h b/fsmonitor.h
> index 7f1794b90b00..f20d72631d76 100644
> --- a/fsmonitor.h
> +++ b/fsmonitor.h
> @@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);
>   */
>  int fsmonitor_is_trivial_response(const struct strbuf *query_result);
>  
> +/*
> + * Check if refresh_fsmonitor has been called at least once.
> + * refresh_fsmonitor is idempotent. Returns true if fsmonitor is
> + * not enabled (since the state will be "fresh" w/ CE_FSMONITOR_VALID unset)
> + * This version is useful for assertions
> + */
> +static inline int is_fsmonitor_refreshed(const struct index_state *istate)
> +{
> +	return !core_fsmonitor || istate->fsmonitor_has_run_once;
> +}
> +
>  /*
>   * Set the given cache entries CE_FSMONITOR_VALID bit. This should be
>   * called any time the cache entry has been updated to reflect the