Re: [GSoC][PATCH 4/5 v3] submodule: port submodule subcommand 'status' from shell to C
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 30, 2017, 23:08 UTC
- Message-ID
- <xmqq60fdoyyt.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170630194727.29787-4-pc44800@gmail.com>
Prathamesh Chavan <pc44800@gmail.com> writes:
Show 6 quoted lines
> + 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 ;-)
Show 18 quoted lines
> + } 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.
Show 8 quoted lines
> + print_status(info, '+', list_item->name, sb.buf,
> + displaypath);
> + strbuf_release(&sb);
> + } else {
> + print_status(info, '+', list_item->name, sub_sha1,
> + displaypath);
> + }
> + }