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

[Junio C Hamano]

Prathamesh Chavan  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.

Looks like a quite straight-forward refactoring.

> +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, ));
> + strbuf_release();
> + return displaypath;

Use of xstrdup() would waste one extra allocation and copy, no?  In
other words, wouldn't this do the same thing?

... {
struct strbuf sb = STRBUF_INIT;

relative_path(path, prefix, );
return strbuf_detach(, NULL);

> @@ -363,7 +375,6 @@ 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();
>   strbuf_addf(, "submodule.%s.active", sub->name);
>   git_config_set_gently(sb.buf, "true");
>   }

This is because sb will stay untouched with the use of the new
helper.  With the way this single strbuf is reused throughout this
function, I cannot help wondering if the prevailing pattern of
resetting and then using is a mistake.  If the strbuf is mostly used
as a scratchpad, wouldn't it make more sense to use it and then
clean up when you are done with it?

I.e. something along this line that shows only two uses...

 builtin/submodule--helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0ff9dd0b85..84ded4b2e9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -363,9 +363,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();
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
+   strbuf_reset();
}
 
/*
@@ -373,7 +373,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();
strbuf_addf(, "submodule.%s.url", sub->name);
if (git_config_get_string(sb.buf, )) {
if (!sub->url)
@@ -410,9 +409,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();
 
/* Copy "update" setting when it is not set yet */
-   strbuf_reset();
strbuf_addf(, "submodule.%s.update", sub->name);
if (git_config_get_string(sb.buf, ) &&
sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {

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

[Prathamesh Chavan]

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 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
As said in the previous update,
a short patch series is floated for the maintainer's review,
and is consisting of the following changes:
* introduce function get_submodule_displaypath()
* introduce function for_each_submodule_list()
* port function set_name_rev() from shell to C
* port submodule subcommand 'status' from shell to C

The complete build report for the above series of patches is available
at:
https://travis-ci.org/pratham-pc/git/builds
Branch: week-14-1
Build #158
The above changes are also push on github and are available at:
https://github.com/pratham-pc/git/commits/week-14-1

The above changes were based on master branch.
But along with the above changes, the same series was also applied
to a separate branch based on the 'next' branch. The changes were
applicable without any additional changes to the patches.
Complete build report of that is also available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: week-14-1-next
Build #159
The above changes are also push on github and are available at:
https://github.com/pratham-pc/git/commits/week-14-1-next

 builtin/submodule--helper.c | 33 ++---
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 84562ec83..dcdbde963 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -220,6 +220,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, ));
+   strbuf_release();
+   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;
@@ -339,16 +360,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 
/* 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, ));
-   else if (get_super_prefix()) {
-   strbuf_addf(, "%s%s", get_super_prefix(), path);
-   displaypath = strbuf_detach(, NULL);
-   } else
-   displaypath = xstrdup(path);
+   displaypath = get_submodule_displaypath(path, prefix);
 
sub = submodule_from_path(_oid, path);
 
@@ -363,7 +375,6 @@ 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();
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
}
-- 
2.13.0