Re: [PATCH v5 1/4] submodule--helper: introduce get_submodule_displaypath()

[Junio C Hamano <gitster@pobox.com>]

Prathamesh Chavan <pc44800@gmail.com> writes:

> Introduce function get_submodule_displaypath() to replace the code
> occurring in submodule_init() for generating displaypath of the
> submodule with a call to it.
>
> This new function will also be used in other parts of the system
> in later patches.
>
> 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 | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 818fe74f0..d24ac9028 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -220,6 +220,29 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
>  	return 0;
>  }
>  
> +/* the result should be freed by the caller. */
> +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 = (len > 0 && is_dir_sep(super_prefix[len - 1])) ? "%s%s" : "%s/%s";
> +
> +		return xstrfmt(format, super_prefix, path);
> +	} else {
> +		return xstrdup(path);
> +	}
> +}

Looks like a fairly faithful rewrite of the original below, with a
reasonable clean-up (e.g. use of xstrfmt() instead of addf()).

One thing I noticed is that future callers of this function, unlike
init_submodule() which is its original caller, are allowed to have
super-prefix that does not end in a slash, because the helper
automatically supplies one when it is missing.

I am not sure the added leniency is desirable [*1*], but in any
case, the added leniency deserves a mention in the log message, I
would think.


[Footnote]

*1* Often it is harder to introduce bugs when the internal rules are
stricter, e.g. "prefix must end with slash", than when they are
looser, e.g. "prefix may or may not end with slash", because
allowing different codepaths to have the same thing in different
representations would hide bugs that is only uncovered when these
codepaths eventually meet (e.g. one codepath has a path with and the
other without trailing slash and they both think that is the
prefix---then they use strcmp() to see if they have the same prefix,
which would be a bug, which may not be noticed for a long time).

>  struct module_list {
>  	const struct cache_entry **entries;
>  	int alloc, nr;
> @@ -335,15 +358,7 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  	struct strbuf sb = STRBUF_INIT;
>  	char *upd = NULL, *url = NULL, *displaypath;
>  
> -	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(path, prefix);
>  
>  	sub = submodule_from_path(&null_oid, path);
>  
> @@ -358,9 +373,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  	 * Set active flag for the submodule being initialized
>  	 */
>  	if (!is_submodule_active(the_repository, path)) {
> -		strbuf_reset(&sb);
>  		strbuf_addf(&sb, "submodule.%s.active", sub->name);
>  		git_config_set_gently(sb.buf, "true");
> +		strbuf_reset(&sb);
>  	}
>  
>  	/*
> @@ -368,7 +383,6 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  	 * To look up the url in .git/config, we must not fall back to
>  	 * .gitmodules, so look it up directly.
>  	 */
> -	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "submodule.%s.url", sub->name);
>  	if (git_config_get_string(sb.buf, &url)) {
>  		if (!sub->url)
> @@ -405,9 +419,9 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
>  				_("Submodule '%s' (%s) registered for path '%s'\n"),
>  				sub->name, url, displaypath);
>  	}
> +	strbuf_reset(&sb);
>  
>  	/* Copy "update" setting when it is not set yet */
> -	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "submodule.%s.update", sub->name);
>  	if (git_config_get_string(sb.buf, &upd) &&
>  	    sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {