git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [GSoC][PATCH 2/6] submodule--helper: introduce get_submodule_displaypath and for_each_submodule_list

From
BWBrandon Williams <bmwill@google.com>
Date
Jun 20, 2017, 18:22 UTC
Message-ID
<20170620182225.GA60134@google.com>
In-Reply-To
<20170619215025.10086-2-pc44800@gmail.com>
On 06/20, Prathamesh Chavan wrote:
Show 5 quoted lines
> 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.

Show 142 quoted lines
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  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
Previous: Prathamesh ChavanNext: Christian Couder
Message 4 of 16 in “[GSoC] Update: Week 5”
  1. Prathamesh ChavanJun 19, 2017
  2. [GSoC][PATCH 1/6] dir: create function count_slashesPrathamesh Chavan, Jun 19, 2017
  3. [GSoC][PATCH 2/6] submodule--helper: introduce get_submodule_displaypath and for_each_submodule_listPrathamesh Chavan, Jun 19, 2017
  4. Brandon WilliamsJun 20, 2017
  5. Christian CouderJun 22, 2017
  6. [GSoC][PATCH 3/6] submodule: port set_name_rev from shell to CPrathamesh Chavan, Jun 19, 2017
  7. [GSoC][PATCH 6/6] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jun 19, 2017
  8. [GSoC][PATCH 4/6] submodule: port submodule subcommand statusPrathamesh Chavan, Jun 19, 2017
  9. Brandon WilliamsJun 20, 2017
  10. [GSoC][PATCH 5/6] submodule: port submodule subcommand sync from shell to CPrathamesh Chavan, Jun 19, 2017
  11. Stefan BellerJun 20, 2017
  12. Christian CouderJun 22, 2017
  13. Stefan BellerJun 20, 2017
  14. Andrew ArdillJun 20, 2017
  15. Brandon WilliamsJun 20, 2017
  16. Prathamesh ChavanJun 26, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.