Re: [PATCH 2/2] Fix git rev-list --bisect and git bisect visualize when the bisection is done in old/new mode.

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 5, 2015, 20:24 UTC
Message-ID: <CAPig+cQ0W_Jw-1b+ghX6N9NYPfb_4KmDnLyCE=SEPAXFrP+egA@mail.gmail.com>
In-Reply-To: <1433522061-14532-2-git-send-email-stuberl@ensimag.grenoble-inp.fr>

On Fri, Jun 5, 2015 at 12:34 PM, Louis Stuber <stuberl@ensimag.grenoble-inp.fr> wrote:

> Fix git rev-list --bisect and git bisect visualize when the bisection
> is done in old/new mode.

See my review of patch 1/2 regarding writing a good commit message. In particular, explain what is broken about "git rev-list --bisect" and "git bisect visualize" so that the reader can understand what this patch is actually fixing.

Show 15 quoted lines

> Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
> Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
> ---
> diff --git a/revision.c b/revision.c
> index 7ddbaa0..b631596 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2075,12 +2075,23 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
>
>  static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>  {
> -       return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
> +       /*
> +        * if BISECT_OLDNEWMODE exists, this is an old/new bisect and the path is different
> +        */

Comments which merely repeat what the code itself already clearly says don't add value, and are thus noise which impede comprehension by distracting the reader from digesting the underlying logic flow.

Show 5 quoted lines

> +       struct stat st;
> +       if (stat(git_path("BISECT_OLDNEWMODE"), &st))
> +               return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
> +       else
> +               return for_each_ref_in_submodule(submodule, "refs/bisect/new", fn, cb_data);

Since the two for_each_ref_in_submodule() calls are identical except for the second argument, the more natural and easier to comprehend way to phrase this would be be to assign "refs/bisect/bad" or "refs/bisect/new" to a variable, and then have just a single invocation of for_each_ref_in_submodule() which uses that variable as its second argument.

Stepping back a moment: My reading of these two patches is that BISECT_OLDNEWMODE is introduced as a simple way to detect if "old/new" mode is being used rather than gleaning that knowledge from the existing BISECT_TERMS file. Is that correct?

If so, then these changes are likely going in the wrong direction. The ominous final sentence of the commit message of patch 1/2 is already a good clue that this approach won't scale well. Further, the approach taken here undesirably emphasizes ease of implementation and its attendant fragility over well thought out design.

Show 15 quoted lines

>  }
>
>  static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>  {
> -       return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
> +       struct stat st;
> +       if (stat(git_path("BISECT_OLDNEWMODE"), &st))
> +               return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
> +       else
> +               return for_each_ref_in_submodule(submodule, "refs/bisect/old", fn, cb_data);
>  }
>
>  static int handle_revision_pseudo_opt(const char *submodule,
> --
> 1.7.1