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.
> 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.
> + 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).
> + 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".
> + return bisect_write(argv[0], argv[1], argv[2], argv[3], nolog);
> default:
> die("BUG: unknown subcommand '%d'", cmdmode);
> }
--
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