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

[Junio C Hamano]

Prathamesh Chavan  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(_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,
> +  _oid, displaypath);
> + goto cleanup;
> + }
> +
> + if (!is_submodule_active(the_repository, list_item->name)) {
> + print_status(info, '-', list_item->name, _item->oid,
> +  displaypath);
> + goto cleanup;
> + }
> +
> + argv_array_pushl(_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, _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(, N_("Suppress submodule status output")),
> + OPT_BOOL(0, "cached", , N_("Use commit stored in the 
> index instead of the one stored in the submodule HEAD")),
> + OPT_BOOL(0, "recursive", , N_("Recurse into nested 
> submodules")),
> + OPT_END()
> + };
> +
> + const char *const git_submodule_helper_usage[] = {
> + N_("git submodule status [--quiet] [--cached] [--recursive] 
> [...]"),
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_status_options,
> +  git_submodule_helper_usage, 0);
> +
> + if (module_list_compute(argc, argv, prefix, , ) < 0)
> + return 1;
> +
> + info.prefix = prefix;
> + info.quiet = !!quiet;
> + info.recursive = !!recursive;
> + info.cached = !!cached;
> +
> + for_each_listed_submodule(, status_submodule, );
> +
> + return 0;
> +}

The same comment as [2/4] on "init_submodule()?  don't you want
init_submodule_cb() instead?" 

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

[Prathamesh Chavan]

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().

The function module_status() acts as the front-end of the subcommand.
It parses subcommand's options and then calls the function
module_list_compute() for computing the list of submodules. Then
this functions calls for_each_listed_submodule() looping through the
list obtained.

Then for_each_listed_submodule() calls submodule_status() for each of the
submodule in its list. The function submodule_status() is responsible
for generating the status each submodule it is called for, and
then calls print_status().

Finally, the function print_status() handles the printing of submodule's
status.

Helper function get_rev_name() removed after porting the above subcommand
as it is no longer used.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 162 +++-
 git-submodule.sh|  49 +-
 2 files changed, 147 insertions(+), 64 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7ca8e8153..8876a4a08 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -293,21 +293,6 @@ static char *compute_rev_name(const char *sub_path, const 
char* object_id)
return NULL;
 }
 
-static int get_rev_name(int argc, const char **argv, const char *prefix)
-{
-   char *revname;
-   if (argc != 3)
-   die("get-rev-name only accepts two arguments:  ");
-
-   revname = compute_rev_name(argv[1], argv[2]);
-   if (revname && revname[0])
-   printf(" (%s)", revname);
-   printf("\n");
-
-   free(revname);
-   return 0;
-}
-
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -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;
+}
+
+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(_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,
+_oid, displaypath);
+   goto cleanup;
+   }
+
+   if (!is_submodule_active(the_repository, list_item->name)) {
+   print_status(info, '-', list_item->name, _item->oid,
+displaypath);
+   goto cleanup;
+   }
+
+   argv_array_pushl(_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)) {
+   print_status(info, ' ', list_item->name, _item->oid,
+displaypath);
+   } else {
+   if (!info->cached) {
+   struct object_id oid;
+
+   if 
(refs_head_ref(get_submodule_ref_store(list_item->name), 
handle_submodule_head_ref, ))
+   die(_("could not resolve HEAD ref inside the"
+ "submodule '%s'"), list_item->name);
+
+   print_status(info, '+', list_item->name, ,
+displaypath);
+   } else {
+   print_status(info, '+', list_item->name,
+