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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 6, 2017, 21:12 UTC
Message-ID: <CAPig+cTEMH=RVfqekuP-oWOoRmNWEdvdFZz4bOdS321oND1Ypg@mail.gmail.com>
In-Reply-To: <20171006132415.2876-2-pc44800@gmail.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:

Show 30 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.
>
> 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...

Show 25 quoted lines

> @@ -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.

Show 24 quoted lines

>         }
>
>         /*
> @@ -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