Prathamesh Chavan <pc44800@xxxxxxxxx> 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 ;-)?
> @@ -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.
> +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?
> +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.
