Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C

[Stephan Beyer <s-beyer@gmx.net>]

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