On 06/20, Prathamesh Chavan wrote:
> Functions get_submodule_displaypath and for_each_submodule_list
> for using them in the later patches, related to porting submodule
> subcommands from shell to C.
> These new functions are also used in ported submodule subcommand
> init
I didn't see anything wrong with these patches, but one small nit is
that this one patch is changing two different things, adding
'get_submodule_displaypath' and 'for_each_submodule_list'. Logically
you could break this patch into two different parts, first introducing
one and then the other.
I'm not saying you need to re-do this patch though (I don't have a super
strong opinion though others might) just wanted to point it out for the
future.
>
> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
> Mentored-by: Stefan Beller <sbeller@xxxxxxxxxx>
> Signed-off-by: Prathamesh Chavan <pc44800@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 69 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 50 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 8cc648d85..f7adca95b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,9 @@
> #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 +222,27 @@ 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;
> + char *displaypath = xstrdup(relative_path(path, prefix, &sb));
> + strbuf_release(&sb);
> + return displaypath;
> + } else if (super_prefix) {
> + int len = strlen(super_prefix);
> + const char *format = is_dir_sep(super_prefix[len-1]) ? "%s%s" : "%s/%s";
> + return xstrfmt(format, super_prefix, path);
> + } else {
> + return xstrdup(path);
> + }
> +}
> +
> struct module_list {
> const struct cache_entry **entries;
> int alloc, nr;
> @@ -330,26 +354,30 @@ static int module_list(int argc, const char **argv, const char *prefix)
> return 0;
> }
>
> -static void init_submodule(const char *path, const char *prefix, int quiet)
> +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);
> +}
> +
> +struct init_cb {
> + const char *prefix;
> + unsigned int quiet: 1;
> +};
> +#define INIT_CB_INIT { NULL, 0 }
> +
> +static void init_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> + struct init_cb *info = cb_data;
> const struct submodule *sub;
> struct strbuf sb = STRBUF_INIT;
> char *upd = NULL, *url = NULL, *displaypath;
>
> - /* Only loads from .gitmodules, no overlay with .git/config */
> - gitmodules_config();
> -
> - if (prefix && get_super_prefix())
> - die("BUG: cannot have prefix and superprefix");
> - else if (prefix)
> - displaypath = xstrdup(relative_path(path, prefix, &sb));
> - else if (get_super_prefix()) {
> - strbuf_addf(&sb, "%s%s", get_super_prefix(), path);
> - displaypath = strbuf_detach(&sb, NULL);
> - } else
> - displaypath = xstrdup(path);
> + displaypath = get_submodule_displaypath(list_item->name, info->prefix);
>
> - sub = submodule_from_path(null_sha1, path);
> + sub = submodule_from_path(null_sha1, list_item->name);
>
> if (!sub)
> die(_("No url found for submodule path '%s' in .gitmodules"),
> @@ -361,7 +389,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
> *
> * Set active flag for the submodule being initialized
> */
> - if (!is_submodule_initialized(path)) {
> + if (!is_submodule_initialized(list_item->name)) {
> strbuf_reset(&sb);
> strbuf_addf(&sb, "submodule.%s.active", sub->name);
> git_config_set_gently(sb.buf, "true");
> @@ -404,7 +432,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
> if (git_config_set_gently(sb.buf, url))
> die(_("Failed to register url for submodule path '%s'"),
> displaypath);
> - if (!quiet)
> + if (!info->quiet)
> fprintf(stderr,
> _("Submodule '%s' (%s) registered for path '%s'\n"),
> sub->name, url, displaypath);
> @@ -433,10 +461,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>
> static int module_init(int argc, const char **argv, const char *prefix)
> {
> + struct init_cb info = INIT_CB_INIT;
> struct pathspec pathspec;
> struct module_list list = MODULE_LIST_INIT;
> int quiet = 0;
> - int i;
>
> struct option module_init_options[] = {
> OPT__QUIET(&quiet, N_("Suppress output for initializing a submodule")),
> @@ -461,8 +489,11 @@ static int module_init(int argc, const char **argv, const char *prefix)
> if (!argc && git_config_get_value_multi("submodule.active"))
> module_list_active(&list);
>
> - for (i = 0; i < list.nr; i++)
> - init_submodule(list.entries[i]->name, prefix, quiet);
> + info.prefix = prefix;
> + info.quiet = !!quiet;
> +
> + gitmodules_config();
> + for_each_submodule_list(list, init_submodule, &info);
>
> return 0;
> }
> --
> 2.13.0
>
--
Brandon Williams
