Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 16, 2016, 07:28 UTC
Message-ID: <CAPig+cS=zcSVLwARZ5A8hdiE0wqgYPU2Jwnr0OcjDXZFO--cZw@mail.gmail.com>
In-Reply-To: <1463031127-17718-4-git-send-email-pranit.bauva@gmail.com>

On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 15 quoted lines

> Reimplement the `write_terms` shell function in C and add a `write-terms`
> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
> remove the subcommand `--check-term-format` as it can now be called from
> inside the function write_terms() C implementation.
>
> Using `--write-terms` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other method.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -56,18 +56,41 @@ static int check_term_format(const char *term, const char *orig_term)
> +int write_terms(const char *bad, const char *good)

s/^/static/

Show 12 quoted lines

> +{
> +       struct strbuf content = STRBUF_INIT;
> +       FILE *fp;
> +       int res;
> +
> +       if (!strcmp(bad, good))
> +               return error(_("please use two different terms"));
> +
> +       if (check_term_format(bad, "bad") || check_term_format(good, "good"))
> +               return -1;
> +
> +       strbuf_addf(&content, "%s\n%s\n", bad, good);

What's the point of the strbuf when you could more easily emit this same content directly to the file via:

    fprintf(fp, "%s\n%s\n", bad, good);

> +       fp = fopen(".git/BISECT_TERMS", "w");

Hardcoding ".git/" is wrong for a variety of reasons: It won't be correct with linked worktrees, or when GIT_DIR is pointing elsewhere, or when ".git" is a symbolic link, etc. Check out the get_git_dir(), get_git_common_dir(), etc. functions in cache.h instead.

> +       if (!fp){

Style: space before '{'

> +               strbuf_release(&content);
> +               die_errno(_("could not open the file to read terms"));

"read terms"? I thought this was writing.

Is dying here correct? I thought we established previously that you should be reporting failure via normal -1 return value rather than dying. Indeed, you're doing so below when strbuf_write() fails.

Show 7 quoted lines

> +       }
> +       res = strbuf_write(&content, fp);
> +       fclose(fp);
> +       strbuf_release(&content);
> +       return (res < 0) ? -1 : 0;
> +}
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)

Style: insert blank line between functions