Re: [PATCH v7 1/3] submodule--helper: introduce get_submodule_displaypath()

[Eric Sunshine]

I didn't find a URL in the cover letter pointing at previous
iterations of this patch series and related discussions, so forgive me
if comments below merely repeat what was said earlier...

On Fri, Oct 6, 2017 at 9:24 AM, Prathamesh Chavan  wrote:
> 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.
>
> Signed-off-by: Prathamesh Chavan 
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -219,6 +219,26 @@ static int resolve_relative_url_test(int argc, const 
> char **argv, const char *pr
> +/* 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, ));
> +   strbuf_release();
> +   return displaypath;
> +   } else if (super_prefix) {
> +   return xstrfmt("%s%s", super_prefix, path);
> +   } else {
> +   return xstrdup(path);
> +   }
> +}

At first glance, this appears to be a simple code-movement patch which
shouldn't require deep inspection by a reviewer, however, upon closer
examination, it turns out that it is doing rather more than that,
which increases reviewer burden, especially since these additional
changes are not mentioned in the commit message. At a minimum, it
includes these changes:

* factors out calls to get_super_prefix()
* adds extra context to the "BUG" message
* changes die("BUG...") to BUG(...)
* allocates/releases a strbuf
* changes assignments to returns

The final two are obvious necessary (or clarifying) changes which a
reviewer would expect to see in a patch which factors code out to its
own function; the others not so.

This isn't to say that the other changes are not reasonable -- they
are -- but if one of your goals is to make the patches easy for
reviewers to digest, then you should make the changes as obvious as
possible for reviewers to spot. One way would be to mention in the
commit message that you're taking the opportunity to also make these
particular cleanups to the code. A more common approach is to place
the various cleanups in preparatory patches before this one, with one
cleanup per patch. I'd prefer to see the latter (if my opinion carries
any weight).

More below...

> @@ -334,15 +354,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, ));
> -   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);
>
> @@ -357,9 +369,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();

This strbuf_reset() movement, and those below, are pretty much just
"noise" changes. They add extra burden to the review process without
really improving the code. The reason they add to reviewer burden is
that they do not seem to be related to the intention stated in the
commit message, so the reviewer must spend extra time trying to
understand their purpose and correctness.

More serious, though, is that these strbuf_reset() movements may
actually increase the burden on someone changing the code in the
future. Presumably, your reason for making these changes is that you
reviewed the code after factoring out the get_submodule_displaypath()
logic and discovered that the strbuf was no longer touched before this
point, therefore resetting it before strbuf_addf() is unnecessary.
While this may be true today, it may not be so in the future. If
someone comes along and adds code above this point which does touch
the strbuf, then these code movements either need to be reverted by

[PATCH v7 1/3] 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 
---
 builtin/submodule--helper.c | 35 +++
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 06ed02f99..56c1c52e2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -219,6 +219,26 @@ 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, ));
+   strbuf_release();
+   return displaypath;
+   } else if (super_prefix) {
+   return xstrfmt("%s%s", super_prefix, path);
+   } else {
+   return xstrdup(path);
+   }
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -334,15 +354,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, ));
-   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);
 
@@ -357,9 +369,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();
}
 
/*
@@ -367,7 +379,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)
@@ -404,9 +415,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) {
-- 
2.14.2