Pranit Bauva <pranit.bauva@xxxxxxxxx> writes:
> +struct bisect_terms {
> + struct strbuf term_good;
> + struct strbuf term_bad;
> +};
I think "struct strbuf" is overrated. For things like this, where
these fields will never change once it is set (and setting it is
done atomically, not incrementally), there is no good reason to use
define the fields as strbuf.
Only because you chose to use strbuf for these two fields, you have
to make unnecessarily copies of argv[] in the command parser, and
you have to remember to discard these copies later.
I think you can just say "const char *" in this case.
> +static int bisect_write(const char *state, const char *rev,
> + const struct bisect_terms *terms, 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, terms->term_bad.buf))
> + strbuf_addf(&tag, "refs/bisect/%s", state);
> + else if (one_of(state, terms->term_good.buf, "skip", NULL))
> + strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
> + else
> + return error(_("Bad bisect_write argument: %s"), state);
OK.
> + if (get_oid(rev, &oid)) {
> + strbuf_release(&tag);
> + return error(_("couldn't get the oid of the rev '%s'"), rev);
> + }
> +
> + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
> + UPDATE_REFS_MSG_ON_ERR)) {
> + strbuf_release(&tag);
> + return -1;
> + }
> + strbuf_release(&tag);
> +
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp)
> + 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);
> + strbuf_release(&commit_name);
> +
> + if (!nolog)
> + fprintf(fp, "git bisect %s %s\n", state, rev);
> +
> + fclose(fp);
> + return 0;
You seem to be _release()ing tag all over the place.
Would it make sense to have a single clean-up label at the end of
function, introduce a "int retval" variable and set it to -1 (or
whatever) when an error is detected and "goto" to the label? It may
make it harder to make such a leak. That is, to end the function
more like:
finish:
if (fp)
fclose(fp);
strbuf_release(&tag);
strbuf_release(&commit_name);
return retval;
}
and have sites with potential errors do something like this:
if (update_ref(...)) {
retval = -1;
goto finish;
}
> + struct bisect_terms terms;
> + bisect_terms_init(&terms);
With the type of "struct bisect_terms" members corrected, you do not
even need the _init() function.
> @@ -182,24 +251,38 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> usage_with_options(git_bisect_helper_usage, options);
>
> switch (cmdmode) {
> + int nolog;
> case NEXT_ALL:
> return bisect_next_all(prefix, no_checkout);
> case WRITE_TERMS:
> if (argc != 2)
> die(_("--write-terms requires two arguments"));
> - return write_terms(argv[0], argv[1]);
> + res = write_terms(argv[0], argv[1]);
> + break;
> case BISECT_CLEAN_STATE:
> if (argc != 0)
> die(_("--bisect-clean-state requires no arguments"));
> - return bisect_clean_state();
> + res = bisect_clean_state();
> + break;
> case BISECT_RESET:
> if (argc > 1)
> die(_("--bisect-reset requires either zero or one arguments"));
> - return bisect_reset(argc ? argv[0] : NULL);
> + res = bisect_reset(argc ? argv[0] : NULL);
> + break;
> case CHECK_EXPECTED_REVS:
> - return check_expected_revs(argv, argc);
> + res = check_expected_revs(argv, argc);
> + break;
> + case BISECT_WRITE:
> + if (argc != 4 && argc != 5)
> + die(_("--bisect-write requires either 4 or 5 arguments"));
> + nolog = (argc == 5) && !strcmp(argv[4], "nolog");
> + strbuf_addstr(&terms.term_good, argv[2]);
> + strbuf_addstr(&terms.term_bad, argv[3]);
Here,
terms.term_good = argv[2];
terms.term_bad = argv[3];
and then you do not need bisect_terms_release() at all.
> + res = bisect_write(argv[0], argv[1], &terms, nolog);
> + break;
> default:
> die("BUG: unknown subcommand '%d'", cmdmode);
> }
> - return 0;
> + bisect_terms_release(&terms);
> + return res;
> }
--
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