git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Prathamesh Chavan <pc44800@gmail.com>
Cc: hanwen@google.com, christian.couder@gmail.com, git@vger.kernel.org, sbeller@google.com
Subject: Re: [PATCH v5 1/4] submodule--helper: introduce get_submodule_displaypath()
Date: Mon, 25 Sep 2017 12:35:18 +0900
Message-ID: <xmqqshfbfo21.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170924120858.26813-2-pc44800@gmail.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) {

  reply index

Thread overview: 59+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-08-21 16:15 [GSoC][PATCH " Prathamesh Chavan
2017-08-21 16:15 ` [GSoC][PATCH 2/4] submodule--helper: introduce for_each_submodule_list() Prathamesh Chavan
2017-08-22 22:37   ` Junio C Hamano
2017-08-21 16:15 ` [GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-08-21 16:47   ` Heiko Voigt
2017-08-21 17:24     ` Prathamesh Chavan
2017-08-21 16:15 ` [GSoC][PATCH 4/4] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-08-22 22:29 ` [GSoC][PATCH 1/4] submodule--helper: introduce get_submodule_displaypath() Junio C Hamano
2017-08-23 18:15   ` [GSoC][PATCH v2 0/4] submodule: Incremental rewrite of git-submodules Prathamesh Chavan
2017-08-23 18:15     ` [GSoC][PATCH v2 1/4] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-08-23 18:15     ` [GSoC][PATCH v2 2/4] submodule--helper: introduce for_each_submodule() Prathamesh Chavan
2017-08-23 19:13       ` Junio C Hamano
2017-08-23 19:31         ` Stefan Beller
2017-08-23 19:52           ` Junio C Hamano
2017-08-24 19:50             ` [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules Prathamesh Chavan
2017-08-24 19:50               ` [GSoC][PATCH v3 1/4] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-08-24 19:50               ` [GSoC][PATCH v3 2/4] submodule--helper: introduce for_each_listed_submodule() Prathamesh Chavan
2017-08-24 19:50               ` [GSoC][PATCH v3 3/4] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-08-24 19:50               ` [GSoC][PATCH v3 4/4] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-08-25 18:51               ` [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules Junio C Hamano
2017-08-25 19:15                 ` Stefan Beller
2017-08-25 20:32                   ` Junio C Hamano
2017-08-27 11:50                 ` Prathamesh Chavan
2017-08-28 11:55                 ` [GSoC][PATCH v4 " Prathamesh Chavan
2017-08-28 11:55                   ` [GSoC][PATCH v4 1/4] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-09-21 15:06                     ` Han-Wen Nienhuys
2017-08-28 11:55                   ` [GSoC][PATCH v4 2/4] submodule--helper: introduce for_each_listed_submodule() Prathamesh Chavan
2017-08-28 11:55                   ` [GSoC][PATCH v4 3/4] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-09-21 15:31                     ` Han-Wen Nienhuys
2017-08-28 11:55                   ` [GSoC][PATCH v4 4/4] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-09-21 16:10                     ` Han-Wen Nienhuys
2017-09-24 12:08                       ` [PATCH v5 0/4] Incremental rewrite of git-submodules Prathamesh Chavan
2017-09-24 12:08                         ` [PATCH v5 1/4] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-09-25  3:35                           ` Junio C Hamano [this message]
2017-09-24 12:08                         ` [PATCH v5 2/4] submodule--helper: introduce for_each_listed_submodule() Prathamesh Chavan
2017-09-25  3:43                           ` Junio C Hamano
2017-09-24 12:08                         ` [PATCH v5 3/4] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-09-25  3:51                           ` Junio C Hamano
2017-09-25  3:55                             ` Junio C Hamano
2017-09-24 12:08                         ` [PATCH v5 4/4] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-09-25  5:06                           ` Junio C Hamano
2017-09-29  9:44                             ` [PATCH v6 0/3] Incremental rewrite of git-submodules Prathamesh Chavan
2017-09-29  9:44                               ` [PATCH v6 1/3] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-10-02  0:44                                 ` Junio C Hamano
2017-09-29  9:44                               ` [PATCH v6 2/3] submodule--helper: introduce for_each_listed_submodule() Prathamesh Chavan
2017-10-02  0:55                                 ` Junio C Hamano
2017-09-29  9:44                               ` [PATCH v6 3/3] submodule: port submodule subcommand 'status' from shell to C Prathamesh Chavan
2017-10-02  1:08                                 ` Junio C Hamano
2017-10-06 13:24                                   ` [PATCH v7 0/3] Incremental rewrite of git-submodules Prathamesh Chavan
2017-10-06 13:24                                     ` [PATCH v7 1/3] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-10-06 21:12                                       ` Eric Sunshine
2017-10-06 13:24                                     ` [PATCH v7 2/3] submodule--helper: introduce for_each_listed_submodule() Prathamesh Chavan
2017-10-06 21:56                                       ` Eric Sunshine
2017-10-06 13:24                                     ` [PATCH v7 3/3] submodule: port submodule subcommand 'status' from shell to C Prathamesh Chavan
2017-10-07  8:51                                     ` [PATCH v7 0/3] Incremental rewrite of git-submodules Junio C Hamano
2017-10-07  9:35                                       ` Eric Sunshine
2017-10-02  0:39                               ` [PATCH v6 " Junio C Hamano
2017-08-23 18:15     ` [GSoC][PATCH v2 3/4] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-08-23 18:15     ` [GSoC][PATCH v2 4/4] submodule: port submodule subcommand 'status' " Prathamesh Chavan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqshfbfo21.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=hanwen@google.com \
    --cc=pc44800@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox