Re: [PATCH v5 4/4] submodule: port submodule subcommand 'status' from shell to C
From: Junio C Hamano <hidden>
Date: 2017-09-25 05:07:07
Possibly related (same subject, not in this thread)
- 2017-09-24 · [PATCH v5 4/4] submodule: port submodule subcommand 'status' from shell to C · Prathamesh Chavan <hidden>
Prathamesh Chavan [off-list ref] 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 ;-)?
quoted hunk
@@ -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.