Skip to content

Commit

Permalink
Use get_oid() instead of spawning a new process
Browse files Browse the repository at this point in the history
Use get_oid() instead of spawning a new process.
Quote from Dscho:

> +static int module_summary(int argc, const char **argv, const char *prefix)
> +{
> +	struct summary_cb info = SUMMARY_CB_INIT;
> +	int cached = 0;
> +	int for_status = 0;
> +	int quiet = 0;
> +	int files = 0;
> +	int summary_limit = -1;
> +	struct child_process cp_rev = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	enum diff_cmd diff_cmd = DIFF_INDEX;
> +	int ret;
> +
> +	struct option module_summary_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of summarising submodules")),
> +		OPT_BOOL(0, "cached", &cached,
> +			 N_("Use the commit stored in the index instead of the submodule HEAD")),
> +		OPT_BOOL(0, "files", &files,
> +			 N_("To compare the commit in the index with that in the submodule HEAD")),
> +		OPT_BOOL(0, "for-status", &for_status,
> +			 N_("Skip submodules with 'ignore_config' value set to 'all'")),
> +		OPT_INTEGER('n', "summary-limit", &summary_limit,
> +			     N_("Limit the summary size")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper summary [<options>] [commit] [--] [<path>]"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_summary_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	if (!summary_limit)
> +		return 0;
> +
> +	cp_rev.git_cmd = 1;
> +	argv_array_pushl(&cp_rev.args, "rev-parse", "-q", "--verify",
> +			 argc ? argv[0] : "HEAD", NULL);

Oy. Why not simply call `get_oid()`? No need to spawn a new process.
------------------------------------------------------

After suggestions from my mentors and doing some digging on a follow-up
mail from Johannes, I figured it out. The problem I faced was that I was
using a 'struct object_id *' instead of just a 'struct object_id' due to
which the assignments and if-statement checks were not coming out to be
very proper. After looking at Dscho's mail, I realised that it will be
better to use the latter struct type since almost everywhere we use this
and the helper functions work better with this. I did some digging in
cache.h to try finding other auxiliary functions to help me. First I
tried to use the oidclr() function to clear the object_id to NULL in
case of when a '--files' is passed but that wasn't the right function to
use (something I realised after concentrating on the definition of the
function really hard).

So here we are, with the fix. All tests pass.
  • Loading branch information
periperidip committed Jul 18, 2020
1 parent 4765017 commit b687677
Showing 1 changed file with 11 additions and 17 deletions.
28 changes: 11 additions & 17 deletions builtin/submodule--helper.c
Expand Up @@ -1260,7 +1260,7 @@ static const char *get_diff_cmd(enum diff_cmd diff_cmd)
}
}

static int compute_summary_module_list(char *head,
static int compute_summary_module_list(struct object_id *head_oid,
struct summary_cb *info,
enum diff_cmd diff_cmd)
{
Expand All @@ -1273,8 +1273,8 @@ static int compute_summary_module_list(char *head,
argv_array_push(&diff_args, "--cached");
argv_array_pushl(&diff_args, "--ignore-submodules=dirty", "--raw",
NULL);
if (head)
argv_array_push(&diff_args, head);
if (head_oid)
argv_array_push(&diff_args, oid_to_hex(head_oid));
argv_array_push(&diff_args, "--");
if (info->argc)
argv_array_pushv(&diff_args, info->argv);
Expand All @@ -1291,7 +1291,7 @@ static int compute_summary_module_list(char *head,
rev.diffopt.format_callback_data = &list;

if (!info->cached) {
if (diff_cmd == DIFF_INDEX)
if (diff_cmd == DIFF_INDEX)
setup_work_tree();
if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
perror("read_cache_preload");
Expand All @@ -1318,9 +1318,8 @@ static int module_summary(int argc, const char **argv, const char *prefix)
int quiet = 0;
int files = 0;
int summary_limit = -1;
struct child_process cp_rev = CHILD_PROCESS_INIT;
struct strbuf sb = STRBUF_INIT;
enum diff_cmd diff_cmd = DIFF_INDEX;
struct object_id head_oid;
int ret;

struct option module_summary_options[] = {
Expand All @@ -1347,25 +1346,21 @@ static int module_summary(int argc, const char **argv, const char *prefix)
if (!summary_limit)
return 0;

cp_rev.git_cmd = 1;
argv_array_pushl(&cp_rev.args, "rev-parse", "-q", "--verify",
argc ? argv[0] : "HEAD", NULL);

if (!capture_command(&cp_rev, &sb, 0)) {
strbuf_strip_suffix(&sb, "\n");
if (!get_oid(argc ? argv[0] : "HEAD", &head_oid)) {
if (argc) {
argv++;
argc--;
}
} else if (!argc || !strcmp(argv[0], "HEAD")) {
/* before the first commit: compare with an empty tree */
strbuf_addstr(&sb, oid_to_hex(the_hash_algo->empty_tree));
oidcpy(&head_oid, the_hash_algo->empty_tree);
if (argc) {
argv++;
argc--;
}
} else {
strbuf_addstr(&sb, "HEAD");
if (get_oid("HEAD", &head_oid))
die(_("could not fetch a revision for HEAD"));
}

if (files) {
Expand All @@ -1383,9 +1378,8 @@ static int module_summary(int argc, const char **argv, const char *prefix)
info.quiet = quiet;
info.summary_limit = summary_limit;

ret = compute_summary_module_list((diff_cmd == DIFF_FILES) ? NULL : sb.buf,
&info, diff_cmd);
strbuf_release(&sb);
ret = compute_summary_module_list((diff_cmd == DIFF_INDEX) ? &head_oid : NULL,
&info, diff_cmd);
return ret;
}

Expand Down

0 comments on commit b687677

Please sign in to comment.