Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Eric,

On Fri, Jun 17, 2016 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement the `bisect_write` shell function in C and add a
>> `bisect-write` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh
>>
>> Using `--bisect-write` subcommand is a temporary measure to port shell
>> function in 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 methods.
>>
>> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
>> from the global shell script thus we need to pass it to the subcommand
>> using the arguments. After the whole conversion, we can remove the extra
>> arguments and make the method use the two variables from the global scope
>> within the C code.
>
> You could do this now rather than waiting for later. Instead of
> passing these arguments to bisect_write(), create global variables in
> this patch and assign them in the BISECT_WRITE case of
> cmd_bisect__helper() before calling bisect_write().
>
> Not necessarily worth a re-roll, but would save you the effort of
> having to explain it here and then change it in some later patch.

I have actually done it in my next conversion which is converting
check_and_set_terms()[1] which also sets those variables to some value
so its more appropriate there.

>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int rev_nr)
>> +static int bisect_write(const char *state, const char *rev,
>> +                       const char *term_good, const char *term_bad,
>> +                       int nolog)
>> +{
>> +       struct strbuf tag = STRBUF_INIT;
>> +       struct strbuf commit_name = STRBUF_INIT;
>> +       struct object_id oid;
>> +       struct commit *commit;
>> +       struct pretty_print_context pp = {0};
>> +       FILE *fp;
>> +
>> +       if (!strcmp(state, term_bad))
>> +               strbuf_addf(&tag, "refs/bisect/%s", state);
>> +       else if(one_of(state, term_good, "skip", NULL))
>> +               strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
>> +       else
>> +               return error(_("Bad bisect_write argument: %s"), state);
>> +
>> +       if (get_oid(rev, &oid)) {
>> +               strbuf_release(&tag);
>> +               return error(_("couldn't get the oid of the rev '%s'"), rev);
>> +       }
>
> Minor: If you move the get_oid() conditional before the one above it,
> then you won't have to worry about releasing 'strbuf tag' at this
> point.

Nice. I will do that. :)

>> +       if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
>> +                      UPDATE_REFS_MSG_ON_ERR)) {
>> +               strbuf_release(&tag);
>> +               return -1;
>> +       }
>
> If you release 'strbuf tag' right here, after it's final use, then you
> won't have to worry about releasing it anywhere below (particularly in
> the error cases).

True.

>> +       fp = fopen(git_path_bisect_log(), "a");
>> +       if (!fp) {
>> +               strbuf_release(&tag);
>> +               return error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
>> +       }
>> +
>> +       commit = lookup_commit_reference(oid.hash);
>> +       format_commit_message(commit, "%s", &commit_name, &pp);
>> +       fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
>> +               commit_name.buf);
>> +
>> +       if (!nolog)
>> +               fprintf(fp, "git bisect %s %s\n", state, rev);
>> +
>> +       strbuf_release(&commit_name);
>> +       strbuf_release(&tag);
>> +       fclose(fp);
>> +       return 0;
>> +}
>> @@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>> +       case BISECT_WRITE:
>> +               if (argc != 4 && argc != 5)
>> +                       die(_("--bisect-write requires either 4 or 5 arguments"));
>> +               nolog = (argc == 5) && !strcmp(argv[4], "nolog");
>
> This is minor and won't matter in the long run when this code goes
> away later in the C conversion, but this differs from the shell code
> which only cared that a (non-empty) fifth argument was provided but
> didn't care about the actual value, whereas this code expects the
> argument to be exactly "nolog".

We currently have tight control over the arguments we are passing as
they are only programmer defined.

>> +               return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
>>         default:
>>                 die("BUG: unknown subcommand '%d'", cmdmode);
>>         }

[1]: https://github.com/pranitbauva1997/git/pull/16

Regards,
Pranit Bauva