[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [GSoC][PATCH 4/5 v3] submodule: port submodule subcommand 'status' from shell to C
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2017-06-30 23:08:42
Message-ID: xmqq60fdoyyt.fsf () gitster ! mtv ! corp ! google ! com
[Download RAW message or body]

Prathamesh Chavan <pc44800@gmail.com> 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);
> +		}
> +	}
[prev in list] [next in list] [prev in thread] [next in thread]