Re: [PATCH] checkout: avoid unncessary match_pathspec calls

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

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

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?