From: Han-Wen Nienhuys <hanwen@google.com>
Date: Thu, 21 Sep 2017 16:10:59 GMT
Subject: [GSoC][PATCH v4 4/4] submodule: port submodule subcommand 'status' from shell to C
Message-ID: <20170921161059.11750-1-hanwen@google.com>
In-Reply-To: <20170828115558.28297-5-pc44800@gmail.com>

+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule status [--quiet] [--cached] [--recursive] [<path>]"),
+		NULL

the manpage over here says

  git submodule [--quiet] status [--cached] [--recursive] [--] [<path>...]

ie. multiple path arguments. Should this usage string be tweaked?

+static void print_status(struct status_cb *info, char state, const char *path,
+			 const struct object_id *oid, const char *displaypath)
+{

could do with a comment. What are the options for the `state` char?

+	if (state == ' ' || state == '+') {
+		struct argv_array get_rev_args = ARGV_ARRAY_INIT;
+
+		argv_array_pushl(&get_rev_args, "get-rev-name",
+				 path, oid_to_hex(oid), NULL);
+		get_rev_name(get_rev_args.argc, get_rev_args.argv,
+			     info->prefix);

since you're not really subprocessing, can't you simply have a

  do_print_rev_name(char *path, char *sha) {
     ..
     printf("\n");
  }

and call that directly? Or call compute_rev_name directly. Then you
don't have to do argv setup here.

Also, the name get_rev_name() is a little unfortunate, since it
doesn't return a name, but rather prints it. Maybe the functions
implementing helper commands could be named like:

  command_get_rev_name

or similar.