Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 6a5878c..1d3e17f 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -24,6 +27,8 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-check-and-set-terms <command> <TERM_GOOD> <TERM_BAD>"),
> N_("git bisect--helper --bisect-next-check [<term>] <TERM_GOOD> <TERM_BAD"),
> N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
> + N_("git bisect--helper --bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
> + "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
Typo: "--bisect start" with space instead of "-"
> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
> return 0;
> }
>
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flags, pathspec_pos, retval = 0;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = STRBUF_INIT;
> + struct strbuf bisect_names = STRBUF_INIT;
> + struct strbuf orig_args = STRBUF_INIT;
> + const char *head;
> + unsigned char sha1[20];
> + FILE *fp = NULL;
> + struct object_id oid;
> +
> + if (is_bare_repository())
> + no_checkout = 1;
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
> + }
> +
> + for (i = 0; i < argc; i++) {
> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> + const char *arg = argv[i];
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
This is without effect since has_double_dash is already set to 1 by the
loop above. I think you can remove this line.
> + break;
> + } else if (!strcmp(arg, "--no-checkout")) {
> + no_checkout = 1;
> + } else if (!strcmp(arg, "--term-good") ||
> + !strcmp(arg, "--term-old")) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(argv[++i]);
> + } else if (skip_prefix(arg, "--term-good=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
> + } else if (skip_prefix(arg, "--term-old=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
I think you can join the last two branches:
+ } else if (skip_prefix(arg, "--term-good=", &arg) ||
+ skip_prefix(arg, "--term-old=", &arg)) {
+ must_write_terms = 1;
+ terms->term_good = xstrdup(arg);
> + } else if (!strcmp(arg, "--term-bad") ||
> + !strcmp(arg, "--term-new")) {
> + must_write_terms = 1;
> + terms->term_bad = xstrdup(argv[++i]);
> + } else if (skip_prefix(arg, "--term-bad=", &arg)) {
> + must_write_terms = 1;
> + terms->term_bad = xstrdup(arg);
> + } else if (skip_prefix(arg, "--term-new=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
This has to be terms->term_bad = ...
Also, you can join the last two branches, again, ie,
+ } else if (skip_prefix(arg, "--term-bad=", &arg) ||
+ skip_prefix(arg, "--term-new=", &arg)) {
+ must_write_terms = 1;
+ terms->term_bad = xstrdup(arg);
> + } else if (starts_with(arg, "--") &&
> + !one_of(arg, "--term-good", "--term-bad", NULL)) {
> + die(_("unrecognised option: '%s'"), arg);
[...]
> + /*
> + * Verify HEAD
> + */
> + head = resolve_ref_unsafe("HEAD", 0, sha1, &flags);
> + if (!head)
> + if (get_sha1("HEAD", sha1))
> + die(_("Bad HEAD - I need a HEAD"));
> +
> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
You were so eager to re-use the comments from the shell script, but you
forgot the "Check if we are bisecting." comment above this line ;-)
> + /* Reset to the rev from where we started */
> + strbuf_read_file(&start_head, git_path_bisect_start(), 0);
> + strbuf_trim(&start_head);
> + if (!no_checkout) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
[...]
> + if (must_write_terms)
> + if (write_terms(terms->term_bad, terms->term_good)) {
> + retval = -1;
> + goto finish;
> + }
> +
bisect_start() is a pretty big function.
I think it can easily be decomposed in some smaller parts, for example,
the following lines ...
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp)
> + return -1;
> +
> + if (fprintf(fp, "git bisect start") < 1) {
> + retval = -1;
> + goto finish;
> + }
> +
> + sq_quote_argv(&orig_args, argv, 0);
> + if (fprintf(fp, "%s", orig_args.buf) < 0) {
> + retval = -1;
> + goto finish;
> + }
> + if (fprintf(fp, "\n") < 1) {
> + retval = -1;
> + goto finish;
> + }
... could be in a function like
static int bisect_append_log(const char **argv)
{
FILE *fp = fopen(git_path_bisect_log(), "a");
struct strbuf orig_args = STRBUF_INIT;
if (!fp)
return -1;
if (fprintf(fp, "git bisect start") < 1) {
retval = -1;
goto finish;
}
sq_quote_argv(&orig_args, argv, 0);
if (fprintf(fp, "%s", orig_args.buf) < 0 ||
fprintf(fp, "\n") < 1) {
retval = -1;
goto finish;
}
finish:
if (fp)
fclose(fp);
strbuf_release(&orig_args);
return retval;
}
and then simply call
retval = bisect_append_log(argv);
in bisect_start()... (This is totally untested.)
If you do not want that for some reason, you should at least fix
> + if (!fp)
> + return -1;
to retval = 1; goto finish; such that the other lists and strings are
released.
> + goto finish;
> +finish:
The "goto finish" right above the "finish" label is unnecessary.
> + if (fp)
> + fclose(fp);
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + strbuf_release(&start_head);
> + strbuf_release(&bisect_names);
> + strbuf_release(&orig_args);
> + return retval;
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
By the way, there are two spaghetti-ish ways to get rid of the
retval = -1;
goto finish;
line pair:
goto fail;
and below the "return retval;" add
fail:
retval = -1;
goto finish;
and you can feel the touch of His Noodly Appendage. *scnr*
The other way is to keep the "goto finish" I deemed unnecessary (right
above the label), and expand it to:
goto finish;
fail:
retval = -1;
finish:
...
Cheers
Stephan