Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Sep 7, 2023, 17:22 UTC
- Message-ID
- <20230907172248.1789451-1-jonathantanmy@google.com>
- In-Reply-To
- <20230907170119.1536694-1-sokcevic@google.com>
Josip Sokcevic <sokcevic@google.com> writes:
Show 7 quoted lines
> 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.
Show 31 quoted lines
> 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.