Hey Stephan,
On Wed, Nov 16, 2016 at 4:49 AM, Stephan Beyer <s-beyer@xxxxxxx> wrote:
> 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.
True. I will 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 = ...
My bad.
> Also, you can join the last two branches, again, ie,
Sure!
> + } 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 ;-)
I will add it back again.
>> + /* 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.)
I think this would be a better choice. Thanks!
> 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*
Nice little trick.
> 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:
> ...
Regards,
Pranit Bauva