Re: [PATCH v5 1/4] submodule--helper: introduce get_submodule_displaypath()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 25, 2017, 03:35 UTC
- Message-ID
- <xmqqshfbfo21.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170924120858.26813-2-pc44800@gmail.com>
Prathamesh Chavan <pc44800@gmail.com> writes:
Show 44 quoted lines
> 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).
Show 50 quoted lines
> 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) {