Prathamesh Chavan <pc44800@xxxxxxxxx> writes:
> + argv_array_pushl(&diff_files_args, "diff-files",
> + "--ignore-submodules=dirty", "--quiet", "--",
> + list_item->name, NULL);
> +
> + if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
> + info->prefix)) {
Essentially we'd only want to run ce_match_stat() on list_item and
the current on-filesystem submodule. We not just know which path we
are interested in, but we already have the cache entry for it, so it
feels quite wasteful to go over the_index.cache[] once again to find
the cache entry that matches list_item->name. Yes, that is how the
scripted Porcelain did it, but it feels way overkill now we have
ready access to the lower level machinery.
Having said all that, I think it is a good idea to stick to a
faithful conversion in this series. I just think a NEEDSWORK
comment may be a good idea to indicate future optimization
opportunities.
A similar optimization is already happening in this patch, actually.
Instead of doing a stat of "$sm_path/.git" and checking test -d/-f,
the code just calls is_submodule_initialized(). Which is good ;-)
> + } else {
> + if (!info->cached) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + prepare_submodule_repo_env(&cp.env_array);
> + cp.git_cmd = 1;
> + cp.dir = list_item->name;
> +
> + argv_array_pushl(&cp.args, "rev-parse",
> + "--verify", "HEAD", NULL);
> +
> + if (capture_command(&cp, &sb, 0))
> + die(_("could not run 'git rev-parse --verify"
> + "HEAD' in submodule %s"),
> + list_item->name);
> +
> + strbuf_strip_suffix(&sb, "\n");
Likewise. This is merely resolving a ref inside a submodule;
calling something like head_ref_submodule() may be sufficient
as an optimization and at least deserves to be mentioned in a
NEEDSWORK comment.
> + print_status(info, '+', list_item->name, sb.buf,
> + displaypath);
> + strbuf_release(&sb);
> + } else {
> + print_status(info, '+', list_item->name, sub_sha1,
> + displaypath);
> + }
> + }
