Re: [PATCH] checkout: avoid unncessary match_pathspec calls

[Junio C Hamano <gitster@pobox.com>]

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

> ---
>  Junio, this patch clearly conflicts wih nd/magic-pathspecs. Do you
>  want me to:
>
>   - hold it off until nd/magic-pathspecs graduates
>   - rebase on top of nd/magic-pathspecs and repost
>   - leave it to you to handle conflicts
>  ?

I'd prefer to take small, independent and clear improvements first
and worry about larger ones later, so if there were another choice,
i.e.

 - eject nd/magic-pathspecs for now, cook this (and other small
   independent and clear improvements you may come up with, some of
   which might come out of nd/magic-pathspecs itself) and let it
   graduate first, and later revisit rerolld nd/magic-pathspecs

that would be the ideal among the given choices ;-).

>  	for (pos = 0; pos < active_nr; pos++) {
>  		struct cache_entry *ce = active_cache[pos];
> +		ce->ce_flags &= ~CE_MATCHED;
>  		if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
>  			continue;
> -		match_pathspec(opts->pathspec, ce->name, ce_namelen(ce), 0, ps_matched);
> +		if (match_pathspec(opts->pathspec, ce->name,
> +				   ce_namelen(ce), 0, ps_matched))
> +			ce->ce_flags |= CE_MATCHED;
>  	}
>  
>  	if (report_path_error(ps_matched, opts->pathspec, opts->prefix))
>  		return 1;
>  
> +	/*
> +	 * call match_pathspec on the remaining entries that have not
> +	 * been done in the previous loop
> +	 */
> +	for (pos = 0; pos < active_nr; pos++) {
> +		struct cache_entry *ce = active_cache[pos];
> +		if (opts->source_tree && !(ce->ce_flags & CE_UPDATE) &&
> +		    match_pathspec(opts->pathspec, ce->name,
> +				   ce_namelen(ce), 0, ps_matched))
> +			ce->ce_flags |= CE_MATCHED;
> +	}
> +

The above is a faithful rewrite, but I have to wonder why you need
two separate loops.

Do you understand what the original loop is doing with ps_matched,
and why the code excludes certain paths while doing so?  I didn't
when I read your patch for the first time, as I forgot, until I
checked with 0a1283bc3955 (checkout $tree $path: do not clobber
local changes in $path not in $tree, 2011-09-30)

You don't use ps_matched after report_path_error(); the new loop
shouldn't have to record which pathspec matched.

Also I notice that I forgot to free ps_matched.  Perhaps doing it
this way is easier to maintain?

	/*
         * Make sure all pathspecs participated in locating the
	 * paths to be checked out.
         */
	for (pos = 0; pos < active_nr; pos++) {
		if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
			/*
                         * "git checkout tree-ish -- path", but this entry
                         * is in the original index; it will not be checked
                         * out to the working tree and it does not matter
			 * if pathspec matched this entry.  We will not do
			 * anything to this entry at all.
                	 */
			verify_psmatch = NULL;
		else
			/*
                         * Either this entry came from the tree-ish
                         * we are checking the paths out of, or we
			 * are checking out of the index.
                	 */
                        verify_psmatch = ps_matched;
		if (match_pathspec(opts->pathspec, ce->name, ce_namelen(ce),
				   0, verify_psmatch))
			ce->ce_flags |= CE_MATCHED;
	}

	if (report_path_error(ps_matched, opts->pathspec, opts->prefix))
		return 1;
	free(ps_matched);

After commenting on the above, it makes me wonder if we even need to
bother marking entries that were in the index that did not come from
the tree-ish we are checking paths out of, though.  What breaks if
you did not do the rewrite above and dropped the second loop in your
patch?