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

[Junio C Hamano <gitster@pobox.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.

>  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.

> +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.

> +
> +	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.