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

[Junio C Hamano <gitster@pobox.com>]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> +			const char **argv, int argc)
> +{
> +	int i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> +	int flag;
> +	struct string_list revs = STRING_LIST_INIT_DUP;
> +	struct string_list states = STRING_LIST_INIT_DUP;
> +	struct strbuf start_head = STRBUF_INIT;
> +	const char *head;
> +	unsigned char sha1[20];
> +	FILE *fp;
> +	struct object_id oid;
> +
> +	if (is_bare_repository())
> +		no_checkout = 1;
> +
> +	for(i = 0; i < argc; i++) {

SP after for.

> +		if (!strcmp(argv[i], "--")) {
> +			has_double_dash = 1;
> +			break;
> +		}
> +		if (!strcmp(argv[i], "--term-good")) {
> +			must_write_terms = 1;
> +			strbuf_reset(&terms->term_good);
> +			strbuf_addstr(&terms->term_good, argv[++i]);
> +			break;
> +		}
> +		if (!strcmp(argv[i], "--term-bad")) {
> +			must_write_terms = 1;
> +			strbuf_reset(&terms->term_bad);
> +			strbuf_addstr(&terms->term_bad, argv[++i]);
> +			break;
> +		}

The original was not careful, either, but what if the user ends the
command line with "--term-good", without anything after it?

Also the original is prepared to handle --term-good=boa; because
this function can be be called directly from the UI (i.e. "git
bisect start --term-good=boa"), not supporting that form would be
seen as a regression.

> +		if (starts_with(argv[i], "--") &&
> +		    !one_of(argv[i], "--term-good", "--term-bad", NULL)) {
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			die(_("unrecognised option: '%s'"), argv[i]);
> +		}
> +		if (get_oid(argv[i], &oid) || has_double_dash) {

Calling get_oid() alone is insufficient to make sure argv[i] refers
to an existing object that is a committish.  The "^{commit}" suffix
in the original is there for a reason.

> +			string_list_clear(&revs, 0);
> +			string_list_clear(&revs, 0);

You seem to want the revs list really really clean ;-)

> +			die(_("'%s' does not appear to be a valid revision"), argv[i]);
> +		}
> +		else
> +			string_list_append(&revs, oid_to_hex(&oid));
> +	}
> +
> +	for (j = 0; j < revs.nr; j++) {

Why "j", not "i", as clearly the previous loop has finished at this
point?  The only reason why replacing "j" with "i" would make this
function buggy would be if a later part of this function depended on
the value of "i" when the control left the above loop, but if that
were the case (I didn't check carefully), such a precious value that
has long term effect throughout the remainder of the function must
not be kept in an otherwise throw-away loop counter variable "i".

Introduce a new "int pathspec_pos" and set it to "i" immediately
after the "for (i = 0; i < argc; i++) { ... }" loop above, perhaps.

> +		struct strbuf state = STRBUF_INIT;
> +		/*
> +		 * The user ran "git bisect start <sha1> <sha1>", hence
> +		 * did not explicitly specify the terms, but we are already
> +		 * starting to set references named with the default terms,
> +		 * and won't be able to change afterwards.
> +		 */
> +		must_write_terms = 1;
> +
> +		if (bad_seen)
> +			strbuf_addstr(&state, terms->term_good.buf);
> +		else {
> +			bad_seen = 1;
> +			strbuf_addstr(&state, terms->term_bad.buf);
> +		}
> +		string_list_append(&states, state.buf);
> +		strbuf_release(&state);
> +	}

How about this instead?

	/*
         * that comment block goes here
         */
       	must_write_terms = !!revs.nr;
	for (i = 0; i < revs.nr; i++) {
                if (bad_seen)
                	string_list_append(&states, terms->term_good.buf);
		else
                	string_list_append(&states, terms->term_bad.buf);
	}

> +
> +	/*
> +	 * Verify HEAD
> +	 */
> +	head = resolve_ref_unsafe("HEAD", 0, sha1, &flag);

The last parameter is a set of flag bits, so call it flags.

> +	if (!head) {
> +		if (get_sha1("HEAD", sha1)) {
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			die(_("Bad HEAD - I need a HEAD"));

We see many repeated calls to clear these two string lists before
exiting with failure, either by dying or return -1.

I wonder how bad the resulting code would look like if we employed
the standard pattern of having a "fail_return:" label at the end of
the function (after the "return" for the usual control flow) to
clear them.  If the result becomes less readable (and I suspect that
you would end up making it less readable), leaving the current code
structure is OK.

> +		}
> +	}
> +	if (!is_empty_or_missing_file(git_path_bisect_start())) {
> +		/* 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;
> +			argv_array_pushl(&argv, "checkout", start_head.buf,
> +					 "--", NULL);
> +			if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> +				error(_("checking out '%s' failed. Try again."),
> +				      start_head.buf);

The original suggests to try "git bisect reset" here to recover.

> +				strbuf_release(&start_head);
> +				string_list_clear(&revs, 0);
> +				string_list_clear(&states, 0);
> +				return -1;
> +			}
> +		}
> +	} else {
> +		if (starts_with(head, "refs/head/") || !get_oid(head, &oid)) {

get_oid() is insufficient to ensure what you have in $head is
40-hex.  I think you meant get_oid_hex() here.

> +			/*
> +			 * This error message should only be triggered by
> +			 * cogito usage, and cogito users should understand
> +			 * it relates to cg-seek.
> +			 */
> +			if (!is_empty_or_missing_file(git_path_head_name())) {
> +				strbuf_release(&start_head);
> +				string_list_clear(&revs, 0);
> +				string_list_clear(&states, 0);
> +				die(_("won't bisect on cg-seek'ed tree"));
> +			}
> +			if (starts_with(head, "refs/heads/")) {

skip_prefix(), perhaps, if "head" is no longer used from here on?

> +	/*
> +	 * Write new start state
> +	 */
> +	fp = fopen(git_path_bisect_start(), "w");
> +	if (!fp) {
> +		bisect_clean_state();
> +		strbuf_release(&start_head);
> +		string_list_clear(&revs, 0);
> +		string_list_clear(&states, 0);
> +		return -1;
> +	}
> +	if (!fprintf(fp, "%s\n", start_head.buf)) {

man 3 fprintf and look for "Return Value"?

> +		fclose(fp);
> +		bisect_clean_state();
> +		strbuf_release(&start_head);
> +		string_list_clear(&revs, 0);
> +		string_list_clear(&states, 0);
> +		return -1;
> +	}
> +	fclose(fp);

Perhaps use write_file() instead of the above block of text?

> +	if (no_checkout) {
> +		get_oid(start_head.buf, &oid);
> +		if (update_ref(NULL, "BISECT_HEAD", oid.hash, NULL, 0,
> +			       UPDATE_REFS_MSG_ON_ERR)) {

Doesn't the original use --no-deref for this update-ref call?

> +			bisect_clean_state();
> +			strbuf_release(&start_head);
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			return -1;
> +		}
> +	}
> +	strbuf_release(&start_head);
> +	fp = fopen(git_path_bisect_names(), "w");
> +
> +	for (; i < argc; i++) {
> +		if (!fprintf(fp, "%s ", argv[i])) {

man 3 fprintf and look for "Return Value"?

More importantly, the original does --sq-quote so that BISECT_NAMES
file can be read back by a shell.  This is important as argv[i] can
have whitespace in it, and you are concatenating them with SP in
between here.  Also you are not terminating that line.

> +			fclose(fp);
> +			bisect_clean_state();
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			return -1;
> +		}
> +	}
> +	fclose(fp);

Perhaps

	strbuf_reset(&bisect_names);
	if (pathspec_pos < argc)
		sq_quote_argv(&bisect_names, argv + pathspec_pos, 0);
	write_file(git_path_bisect_names(), "%s\n", bisect_names.buf);

or something like that?

> +	for (j = 0; j < states.nr; j ++) {

Again, is "i" still precious here?  Style: drop SP between j and ++.

> +	fp = fopen(git_path_bisect_log(), "a");
> +	if (!fp) {
> +		bisect_clean_state();
> +		return -1;
> +	}
> +	if (!fprintf(fp, "git bisect start")) {
> +		bisect_clean_state();
> +		return -1;
> +	}
> +	for (i = 0; i < argc; i++) {
> +		if (!fprintf(fp, " '%s'", argv[i])) {
> +			fclose(fp);
> +			bisect_clean_state();
> +			return -1;
> +		}
> +	}
> +	if (!fprintf(fp, "\n")) {
> +		fclose(fp);
> +		bisect_clean_state();
> +		return -1;
> +	}

Again, the original writes orig_args which was protected with --sq-quote.

> +	fclose(fp);
> +
> +	return 0;
> +}
> +