Hey Eric,
On Fri, Jun 17, 2016 at 12:25 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
--
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