Nguyễn Thái Ngọc Duy <[email protected]> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
