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

[Eric Sunshine <sunshine@sunshineco.com>]

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 <pc44800@gmail.com> 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 <pc44800@gmail.com>
> ---
> 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, &sb));
> +               strbuf_release(&sb);
> +               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, &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);
>
> @@ -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(&sb);
>                 strbuf_addf(&sb, "submodule.%s.active", sub->name);
>                 git_config_set_gently(sb.buf, "true");
> +               strbuf_reset(&sb);

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
that person (more noise) or that person needs to remember to add a
strbuf_reset() at the end of the new code.

Moreover, it's somewhat easier to reason about the strbuf_reset()'s
and the corresponding strbuf_addf()'s when they are kept together, as
in the original code, so, for that reason alone, one could argue that
moving the strbuf_reset()'s does not really improve the code.

I'd suggest dropping these changes in the re-roll.

>         }
>
>         /*
> @@ -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(&sb);
>         strbuf_addf(&sb, "submodule.%s.url", sub->name);
>         if (git_config_get_string(sb.buf, &url)) {
>                 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(&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) {
> --
> 2.14.2