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

From: Pranit Bauva <pranit.bauva@gmail.com>
Date: May 20, 2016, 07:42 UTC
Message-ID: <CAFZEwPPp+CWpnW-RMz5aYsgYDfdVVSZqCCXQQp0z=uC5QE140g@mail.gmail.com>
In-Reply-To: <CAPig+cS=zcSVLwARZ5A8hdiE0wqgYPU2Jwnr0OcjDXZFO--cZw@mail.gmail.com>

Hey Eric,

On Mon, May 16, 2016 at 12:58 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 18 quoted lines

> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> 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/

Sure. Will include this in re-roll.

Show 17 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);

fprintf seems way better and it is also extensively used in git's source code. Will update.

Show 10 quoted lines

>> +       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 '{'

Sorry. Might have missed this. Will include this in re-roll

>> +               strbuf_release(&content);
>> +               die_errno(_("could not open the file to read terms"));
>
> "read terms"? I thought this was writing.

Ya. It should be "write terms".

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

I was confused about this. I thought not able to open a file is an exceptional situation (because it has never happened with me) and thus I used die(). I will use error() as further justified by Johannes.

Show 9 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

Sure.

Regards, Pranit Bauva