Re: [PATCH v5 4/4] submodule: port submodule subcommand 'status' from shell to C

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2017, 05:06 UTC
Message-ID: <xmqq4lrrfjt9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170924120858.26813-5-pc44800@gmail.com>

Prathamesh Chavan <pc44800@gmail.com> writes:

> This aims to make git-submodule 'status' a built-in. Hence, the function
> cmd_status() is ported from shell to C. This is done by introducing
> three functions: module_status(), submodule_status() and print_status().

Well then it does not just aim to, but it does, doesn't it ;-)?

Show 36 quoted lines

> @@ -559,6 +544,151 @@ static int module_init(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct status_cb {
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +	unsigned int recursive: 1;
> +	unsigned int cached: 1;
> +};
> +#define STATUS_CB_INIT { NULL, 0, 0, 0 }
> +
> +static void print_status(struct status_cb *info, char state, const char *path,
> +			 const struct object_id *oid, const char *displaypath)
> +{
> +	if (info->quiet)
> +		return;
> +
> +	printf("%c%s %s", state, oid_to_hex(oid), displaypath);
> +
> +	if (state == ' ' || state == '+')
> +		printf(" (%s)", compute_rev_name(path, oid_to_hex(oid)));
> +
> +	printf("\n");
> +}
> +
> +static int handle_submodule_head_ref(const char *refname,
> +				     const struct object_id *oid, int flags,
> +				     void *cb_data)
> +{
> +	struct object_id *output = cb_data;
> +	if (oid)
> +		oidcpy(output, oid);
> +
> +	return 0;
> +}

All of the above look sensible.

Show 30 quoted lines

> +static void status_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +	struct status_cb *info = cb_data;
> +	char *displaypath;
> +	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
> +
> +	if (!submodule_from_path(&null_oid, list_item->name))
> +		die(_("no submodule mapping found in .gitmodules for path '%s'"),
> +		      list_item->name);
> +
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +	if (ce_stage(list_item)) {
> +		print_status(info, 'U', list_item->name,
> +			     &null_oid, displaypath);
> +		goto cleanup;
> +	}
> +
> +	if (!is_submodule_active(the_repository, list_item->name)) {
> +		print_status(info, '-', list_item->name, &list_item->oid,
> +			     displaypath);
> +		goto cleanup;
> +	}
> +
> +	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)) {

Calling any cmd_foo() from places other than git.c, especially if it is done more than once, is a source of bug. I didn't check closely if cmd_diff_files() currently has any of the potential problems, but these functions are free to (1) modify global or file-scope static variables, assuming that they will not be called again, leaving these variables in a state different from the original and making cmd_foo() unsuitable to be called twice, and (2) exit instead of return at the end, among other things.

The functions in diff-lib.c (I think run_diff_files() for this application) are designed to be called multiple times as library-ish helpers; this caller should be using them instead.

> +		print_status(info, ' ', list_item->name, &list_item->oid,
> +			     displaypath);
> +	} else {
> +		if (!info->cached) {

By using "else if (!info->cached) {" here, you can reduce the nesting level, perhaps?

Show 36 quoted lines

> +static int module_status(int argc, const char **argv, const char *prefix)
> +{
> +	struct status_cb info = STATUS_CB_INIT;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int cached = 0;
> +	int recursive = 0;
> +
> +	struct option module_status_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
> +		OPT_BOOL(0, "cached", &cached, N_("Use commit stored in the index instead of the one stored in the submodule HEAD")),
> +		OPT_BOOL(0, "recursive", &recursive, N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>...]"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_status_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> +		return 1;
> +
> +	info.prefix = prefix;
> +	info.quiet = !!quiet;
> +	info.recursive = !!recursive;
> +	info.cached = !!cached;
> +
> +	for_each_listed_submodule(&list, status_submodule, &info);
> +
> +	return 0;
> +}

The same comment as [2/4] on "init_submodule()? don't you want init_submodule_cb() instead?" applies to "status_submodule()". I suspect that in the future we would want more direct calls to show the status of one single submodule, without indirectly controlling what is chosen via the &list interface, and if that is the case, you'd want the interface into the leaf level helper function to be a more direct one that is not based on "void *cb_data", with a thin wrapper around it that is to be used as the callback function by for_each_listed_submodule().

Other than that, looks very cleanly done.

Thanks.