Hey Eric,
On Mon, May 16, 2016 at 12:58 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
>> +{
>> + 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.
>> + 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.
>> + }
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html