Re: [PATCH v3 1/1] 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:

> +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)++;

We tried to check out ce and found out that the path is already
occupied, whose stat data is in st.  We increment (*count)++ to
signal the fact that ce itself had trouble.

> +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */
> +	for (i = 0; i < state->istate->cache_nr; i++) {

Do we want to count up all the way?  IOW, don't we want to stop when
we see the ce itself?

> +		struct cache_entry *dup = state->istate->cache[i];
> +
> +		if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE))
> +			continue;
> +
> +		if (ce_uptodate(dup) &&
> +		    dup->ce_stat_data.sd_ino == st->st_ino) {
> +			dup->ce_flags |= CE_MATCHED;

Is checking ce_uptodate(dup) done to check that the 'dup' is a
freshly checked out entry?  I'd like to understand why it is
necessary, especially beause you know you only call this function in
the o->clone codepath, so everything ought to be fresh.

I agree that the check you have above is sufficient on inum capable
systems.  I also suspect that Windows folks, if they really wish to
report which other path(s) are colliding with ce, would want to
replace this whole loop with a platform specific helper function,
not just the condition in the above "if" statement [*1*], so what we
see here should be sufficient for now.

	Side note #1.  It is sufficient for inum capable systems to
	look at 'dup' and 'st' to cheaply identify collision, but it
	may be more efficient for other systems to look at ce and
	the previous entries of the index, which allows them to use
	platform specific API that knows case-folding and other
	pathname munging specifics.  Going that way lets them
	minimally emulate 'struct stat', which may be expensive to
	fill.

> +			(*count)++;

And then we increment (*count)++ for this _other_ one that
collided with ce.

> +			break;

And we leave the loop.  There may be somebody else that collided
when 'dup' was checked out, making this triple collision, but in
such a case, we won't be coming this far in this loop (i.e.  we
would have continued on this 'dup' as it is marked as CE_MATCHED),
so there is no reason to continue looking for further collision with
'ce' here.  So nr_duplicates kept track by these (*count)++ will
indicate number of paths involved in any collision correctly, I
think.

Makes sense.

> +		}
> +	}
> +#endif
> +}
> +
>  /*
>   * Write the contents from ce out to the working tree.
>   *
> @@ -455,6 +484,9 @@ int checkout_entry(struct cache_entry *ce,
>  			return -1;
>  		}
>  
> +		if (state->clone)
> +			mark_duplicate_entries(state, ce, &st);
> +
>  		/*
>  		 * We unlink the old file, to get the new one with the
>  		 * right permissions (including umask, which is nasty
> diff --git a/unpack-trees.c b/unpack-trees.c
> index f9efee0836..d4fece913c 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -344,12 +344,20 @@ static int check_updates(struct unpack_trees_options *o)
>  	struct index_state *index = &o->result;
>  	struct checkout state = CHECKOUT_INIT;
>  	int i;
> +	int nr_duplicates = 0;
>  
>  	state.force = 1;
>  	state.quiet = 1;
>  	state.refresh_cache = 1;
>  	state.istate = index;
>  
> +	if (o->clone) {
> +		state.clone = 1;
> +		state.nr_duplicates = &nr_duplicates;
> +		for (i = 0; i < index->cache_nr; i++)
> +			index->cache[i]->ce_flags &= ~CE_MATCHED;
> +	}
> +
>  	progress = get_progress(o);
>  
>  	if (o->update)
> @@ -414,6 +422,20 @@ static int check_updates(struct unpack_trees_options *o)
>  	errs |= finish_delayed_checkout(&state);
>  	if (o->update)
>  		git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
> +
> +	if (o->clone && state.nr_duplicates) {

Does state.nr_duplicates have to be a pointer to int?  I am
wondering if state.clone bit is sufficient to protect codepaths that
needs to spend cycles to keep track of that number, in which case we
can do without local variable nr_duplicates here, and the set-up
code in the previous hunk not to store the pointer to it in
state.nr_duplicates (instead, that field itself can become an int).

> +		warning(_("the following paths have collided and only one from the same\n"
> +			  "colliding group is in the working tree:\n"));

As we are not grouping (and do not particularly see the need to
group), perhaps we can do without mentioning group in the warning?
e.g.

	some of the following may have been checked out incorrectly
	due to limitation of the filesystem like case insensitivity.


> +		for (i = 0; i < index->cache_nr; i++) {
> +			struct cache_entry *ce = index->cache[i];
> +
> +			if (!(ce->ce_flags & CE_MATCHED))
> +				continue;
> +			fprintf(stderr, "  '%s'\n", ce->name);
> +			ce->ce_flags &= ~CE_MATCHED;
> +		}
> +	}