Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C
From: Eric Sunshine <hidden>
Date: 2016-06-16 18:55:16
On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva [off-list ref] 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.
quoted hunk ↗ jump to hunk
Signed-off-by: Pranit Bauva <redacted> ---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).
quoted hunk ↗ jump to hunk
+ 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);
}