Re: [PATCH v14 09/27] bisect--helper: `bisect_write` shell function in C
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 24, 2016, 22:30 UTC
- Message-ID
- <xmqqfuptol4h.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <01020156b73fe6a7-2e9df745-e3f4-4830-a1af-4acae7964c11-000000@eu-west-1.amazonses.com>
Pranit Bauva <pranit.bauva@gmail.com> 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.
Show 16 quoted lines
> +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.
Show 27 quoted lines
> + 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.
Show 35 quoted lines
> @@ -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.
Show 9 quoted lines
> + 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;
> }