Re: [PATCH] checkout: avoid unncessary match_pathspec calls

Nguyễn Thái Ngọc Duy [off-list ref] 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?