Pranit Bauva [off-list ref] 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.
quoted hunk
@@ -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;
}
Hey Junio,
On Thu, Aug 25, 2016 at 4:00 AM, Junio C Hamano [off-list ref] wrote:
Pranit Bauva [off-list ref] writes:
quoted
+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.
quoted
+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.
quoted
+ 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;
}
quoted
+ 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.
quoted
@@ -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.
quoted
+ 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;
}