Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on

[Jonathan Tan <jonathantanmy@google.com>]

Josip Sokcevic <sokcevic@google.com> writes:
> git-diff-index may return incorrect deleted entries when fsmonitor is used in a
> repository with git submodules. This can be observed on Mac machines, but it
> can affect all other supported platforms too.
> 
> If fsmonitor is used, `stat *st` may not be initialized. Since `lstat` calls
> aren't not desired when fsmonitor is on, skip the entire gitlink check using
> the same condition used to initialize `stat *st`.

I think this paragraph is outdated - you'll need to update it to match
the code in this version.

> diff --git a/diff-lib.c b/diff-lib.c
> index d8aa777a73..664613bb1b 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -39,11 +39,22 @@
>  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;
> -		return 1;
> +	if (ce->ce_flags & CE_FSMONITOR_VALID) {
> +		/*
> +		 * Both check_removed() and its callers expect lstat() to have
> +		 * happened and, in particular, the st_mode field to be set.
> +		 * Simulate this with the contents of ce.
> +		 */
> +		memset(st, 0, sizeof(*st));
> +		st->st_mode = ce->ce_mode;
> +	} else {
> +		if (lstat(ce->name, st) < 0) {
> +			if (!is_missing_file_error(errno))
> +				return -1;
> +			return 1;
> +		}
>  	}
> +
>  	if (has_symlink_leading_path(ce->name, ce_namelen(ce)))
>  		return 1;
>  	if (S_ISDIR(st->st_mode)) {

I'm on the fence about whether the extra newline is necessary - the "if"
did get bigger, but the code is clear enough without the newline. I lean
towards removing it, to avoid cluttering the diff.