Re: [PATCH v5] 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:

>  I still don't trust magic st_ino zero, or core.checkStat being zero
>  on Windows, so the #if condition still remains but it covers smallest
>  area possible and I tested it by manually make it "#if 1"
>
>  The fallback with fspathcmp() is only done when inode can't be
>  trusted because strcmp is more expensive and when fspathcmp() learns
>  more about real world in the future, it could become even more
>  expensive.
>
>  The output sorting is the result of Sublime-Gitignore repo being
>  reported recently. It's not perfect but it should help seeing the
>  groups in normal case.

Looks small and safe.

> +
> +	if (o->clone) {
> +		struct string_list list = STRING_LIST_INIT_NODUP;
> +		int i;
> +
> +		for (i = 0; i < index->cache_nr; i++) {
> +			struct cache_entry *ce = index->cache[i];
> +
> +			if (!(ce->ce_flags & CE_MATCHED))
> +				continue;
> +
> +			string_list_append(&list, ce->name);
> +			ce->ce_flags &= ~CE_MATCHED;
> +		}
> +
> +		list.cmp = fspathcmp;
> +		string_list_sort(&list);
> +
> +		if (list.nr)
> +			warning(_("the following paths have collided (e.g. case-sensitive paths\n"
> +				  "on a case-insensitive filesystem) and only one from the same\n"
> +				  "colliding group is in the working tree:\n"));
> +
> +		for (i = 0; i < list.nr; i++)
> +			fprintf(stderr, "  '%s'\n", list.items[i].string);
> +
> +		string_list_clear(&list, 0);

I would have written the "sort, show warning, and list" all inside
"if (list.nr)" block, leaving list-clear outside, which would have
made the logic a bit cleaner.  The reader does not have to bother
thinking "ah, when list.nr==0, this is a no-op anyway" to skip them
if written that way.

I highly suspect that the above was written in that way to reduce
the indentation level, but the right way to reduce the indentation
level, if it bothers readers too much, is to make the whole thing
inside the above if (o->clone) into a dedicated helper function
"void report_collided_checkout(void)", I would think.