Hey Junio,
On Thu, Aug 25, 2016 at 4:00 AM, Junio C Hamano <[email protected]> wrote:
> Pranit Bauva <[email protected]> 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.
Using struct strbuf is not really overrated but in fact required. But
yes, for this patch it might seem as overrated. In the shell code
initally TERM_GOOD is set to "good" while TERM_BAD is set to "bad".
Now there are a lot of instances (one of which is bisect_start()
function) where this can change. So if we keep it as "const char *",
it would be right to change the value of it after wards. And we cannot
keep it as "char []" because we don't know its size before hand.
>> +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:
I think I could use goto for this function.
> 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.
Discussed above.
>> @@ -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.
Discussed above.
>> + 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
