Re: [PATCH v2] clone: report duplicate entries on case-insensitive filesystems

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2018, 19:31 UTC
Message-ID: <xmqq7el2km82.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180807190110.16216-1-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

>  One nice thing about this is we don't need platform specific code for
>  detecting the duplicate entries. I think ce_match_stat() works even
>  on Windows. And it's now equally expensive on all platforms :D

ce_match_stat() may not be a very good measure to see if two paths refer to the same file, though. After a fresh checkout, I would not be surprised if two completely unrelated paths have the same size and have same mtime/ctime. In its original use case, i.e. "I have one specific path in mind and took a snapshot of its metadata earlier. Is it still the same, or has it been touched?", that may be sufficient to detect that the path has been _modified_, but without reliable inum, it may be a poor measure to say two paths refer to the same.

Show 6 quoted lines

>  builtin/clone.c |  1 +
>  cache.h         |  2 ++
>  entry.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  unpack-trees.c  | 23 +++++++++++++++++++++++
>  unpack-trees.h  |  1 +
>  5 files changed, 71 insertions(+)

Having said that, it is pleasing to see that this can be achieved with so little additional code.

Show 41 quoted lines

> +static void mark_duplicate_entries(const struct checkout *state,
> +				   struct cache_entry *ce, struct stat *st)
> +{
> +	int i;
> +	int *count = state->nr_duplicates;
> +
> +	if (!count)
> +		BUG("state->nr_duplicates must not be NULL");
> +
> +	ce->ce_flags |= CE_MATCHED;
> +	(*count)++;
> +
> +	if (!state->refresh_cache)
> +		BUG("We need this to narrow down the set of updated entries");
> +
> +	for (i = 0; i < state->istate->cache_nr; i++) {
> +		struct cache_entry *dup = state->istate->cache[i];
> +
> +		/*
> +		 * This entry has not been checked out yet, otherwise
> +		 * its stat info must have been updated. And since we
> +		 * check out from top to bottom, the rest is guaranteed
> +		 * not checked out. Stop now.
> +		 */
> +		if (!ce_uptodate(dup))
> +			break;
> +
> +		if (dup->ce_flags & CE_MATCHED)
> +			continue;
> +
> +		if (ce_match_stat(dup, st,
> +				  CE_MATCH_IGNORE_VALID |
> +				  CE_MATCH_IGNORE_SKIP_WORKTREE))
> +			continue;
> +
> +		dup->ce_flags |= CE_MATCHED;
> +		(*count)++;
> +		break;
> +	}
> +}
> +

Hmph. If there is only one true collision, then all its aliases will be marked with CE_MATCHED bit every time the second and the subsequent alias is checked out (as the caller calls this function when it noticed that something already is at the path ce wants to go). But if there are two sets of colliding paths, because there is only one bit used, we do not group the paths into these two sets and report, e.g. "blue.txt, BLUE.txt and BLUE.TXT collide. red.txt and RED.txt also collide." I am not sure if computing that is too much work for too little gain, but because this is in an error codepath, it may be worth doing. I dunno.

Show 5 quoted lines

> +
> +	if (o->clone && state.nr_duplicates) {
> +		warning(_("the following paths in this repository only differ in case\n"
> +			  "from another path and will cause problems because you have cloned\n"
> +			  "it on an case-insensitive filesytem:\n"));

With the new approach, we no longer preemptively detect that the project will be harmed by a case smashing filesystems before it happens. This instead reports that the project has already been harmed on _this_ system by such a filesystem after the fact.

So from the end-user's point of view, "will cause problems" may be a message that came a bit too late. "have collided and only one from the same colliding group is in the working tree; others failed to be checked out" is probably closer to the truth.