On Sun, May 21, 2017 at 5:27 AM, Prathamesh Chavan <pc44800@xxxxxxxxx> wrote:
> This aims to make git-submodule status a builtin. 'status' is ported
> to submodule--helper, and submodule--helper is called from
> git-submodule.sh.
>
> For the purpose of porting cmd_status, the code is split up such that
> one function obtains all the list of submodules, acting as the
> front-end of git-submodule status. This function later calls the
> second function for_each_submodule_list,it which basically loops
> through the list of submodules and calls function fn, which in this
> case is status_submodule. The third function, status submodule returns
> the status of submodule and also takes care of the recursive flag.
>
> The first function module_status parses the options present in argv,
> and then with the help of module_list_compute, generates the list of
> submodules present in the current working tree.
>
> The second function for_each_submodule_list traverses through the list,
> and calls function fn (which in the case of submodule subcommand
> foreach is status_submodule) is called for each entry.
>
> The third function status_foreach checks for the various conditions,
> and prints the status of the submodule accordingly. Also, this
> function takes care of the recursive flag by creating a separate
> child_process and running it inside the submodule.
>
> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
> Mentored-by: Stefan Beller <sbeller@xxxxxxxxxx>
> Signed-off-by: Prathamesh Chavan <pc44800@xxxxxxxxx>
> ---
> A new function, get_submodule_displaypath is also introduced for getting
> the displaypath of the submodule while taking care of its prefix and
> superprefix.
>
> builtin/submodule--helper.c | 162 ++++++++++++++++++++++++++++++++++++++++++++
> git-submodule.sh | 48 +------------
> 2 files changed, 163 insertions(+), 47 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5f0ddd8ad..7c040a375 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,8 @@
> #include "refs.h"
> #include "connect.h"
>
> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, void *cb_data);
> +
> static char *get_default_remote(void)
> {
> char *dest = NULL, *ret;
> @@ -219,6 +221,23 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
> return 0;
> }
>
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> + const char *super_prefix = get_super_prefix();
> +
> + if (prefix && super_prefix) {
> + BUG("cannot have prefix '%s' and superprefix '%s'",
> + prefix, super_prefix);
> + } else if (prefix) {
> + struct strbuf sb = STRBUF_INIT;
> + return xstrdup(relative_path(path, prefix, &sb));
> + } else if (super_prefix) {
> + return xstrfmt("%s/%s", super_prefix, path);
> + } else {
> + return xstrdup(path);
> + }
> +}
> +
> enum describe_step {
> step_bare = 0,
> step_tags,
> @@ -395,6 +414,13 @@ static int module_list(int argc, const char **argv, const char *prefix)
> return 0;
> }
>
> +static void for_each_submodule_list(const struct module_list list, submodule_list_func_t fn, void *cb_data)
> +{
> + int i;
> + for (i = 0; i < list.nr; i++)
> + fn(list.entries[i], cb_data);
> +}
Up to here it looks like the patch in
https://public-inbox.org/git/20170521125814.26255-2-pc44800@xxxxxxxxx/
(without the nit of having an extra void return)
Maybe it is worth it to combine the two patch series, such that we'd need
to review the common parts only once?
> +
> static void init_submodule(const char *path, const char *prefix, int quiet)
> {
> const struct submodule *sub;
> @@ -532,6 +558,141 @@ static int module_init(int argc, const char **argv, const char *prefix)
> return 0;
> }
>
> +struct cb_status {
> + const char *prefix;
> + unsigned int quiet: 1;
> + unsigned int cached: 1;
> + unsigned int recursive: 1;
> +};
> +#define CB_STATUS_INIT { NULL, 0, 0, 0 }
> +
> + if (run_command(&cpr))
> + die(_("Failed to recurse into submodule path %s"), list_item->name);
I thought this is a badly worded error message, but it turns out it
is just as in the shell code, which is good for a direct translation.
Maybe we can adapt the error message in a later follow up to be more
aligned to other submodule error messages. (dropping "path" and putting
single quotes around %s, also un-capitalize the first letter)
> +static int module_status(int argc, const char **argv, const char *prefix)
> +{
> + struct cb_status info = CB_STATUS_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 output for initializing a submodule")),
> + 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.cached = !!cached;
> + info.recursive = !!recursive;
> +
> + for_each_submodule_list(list, status_submodule, &info);
> +
> + return 0;
> +}
This function looks good. Though my gut reaction was to suggest to
add another layer of abstraction. Then I checked wt-status.c, but that
makes use of "submodule summary" and not "submodule status". So all is good.
> + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper status ${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive} "$@"
I'd think we would not need to pass down superprefix here as we do not call
"submodule status" in a recursive way. The recursion works on
the submodule helper itself, so we could simplify it to just
git ${wt_prefix:+-C "$wt_prefix"} submodule--helper status
${GIT_QUIET:+--quiet} ${cached:+--cached} ${recursive:+--recursive}
"$@"
Another idea that I just had:
Maybe we could drop --cached, --recursive as well,
as they are just command line options, which could
be just part of "$@".
For --quiet this is a bit more complicated as it may come in
via an environment variable (which we could also check for
in C in theory. I know I omitted that when writing some
submodule--helper code a couple months ago, but the reason
escaped me)
Thanks,
Stefan