git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 2, 2016, 20:19 UTC
Message-ID
<xmqqr3a6divf.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<0102015640423d18-86d50542-5e5d-40ff-860e-caf56146982d-000000@eu-west-1.amazonses.com>
Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 17 quoted lines
> +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.
Show 16 quoted lines
> +		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.

Show 7 quoted lines
> +		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 ;-)
Show 7 quoted lines
> +			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.

Show 18 quoted lines
> +		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);
	}
Show 5 quoted lines
> +
> +	/*
> +	 * Verify HEAD
> +	 */
> +	head = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
The last parameter is a set of flag bits, so call it flags.
Show 5 quoted lines
> +	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.

Show 13 quoted lines
> +		}
> +	}
> +	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.
Show 8 quoted lines
> +				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.

Show 12 quoted lines
> +			/*
> +			 * 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?
Show 12 quoted lines
> +	/*
> +	 * 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"?
Show 8 quoted lines
> +		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?
Show 12 quoted lines
> +			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.

Show 8 quoted lines
> +			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 ++.
Show 21 quoted lines
> +	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.
Show 5 quoted lines
> +	fclose(fp);
> +
> +	return 0;
> +}
> +
Previous: Pranit BauvaNext: Pranit Bauva
Message 30 of 39 in “bisect--helper: use OPT_CMDMODE instead of OPT_BOOL”
  1. 01/13 bisect--helper: use OPT_CMDMODE instead of OPT_BOOLPranit Bauva, Jul 31, 2016
  2. 06/13 wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()Pranit Bauva, Jul 31, 2016
  3. 05/13 t6030: explicitly test for bisection cleanupPranit Bauva, Jul 31, 2016
  4. 03/13 bisect--helper: `write_terms` shell function in CPranit Bauva, Jul 31, 2016
  5. Junio C HamanoAug 2, 2016
  6. Pranit BauvaAug 3, 2016
  7. Junio C HamanoAug 4, 2016
  8. 07/13 bisect--helper: `bisect_reset` shell function in CPranit Bauva, Jul 31, 2016
  9. Junio C HamanoAug 2, 2016
  10. 11/13 bisect--helper: `bisect_next_check` shell function in CPranit Bauva, Jul 31, 2016
  11. Junio C HamanoAug 2, 2016
  12. Pranit BauvaAug 3, 2016
  13. 12/13 bisect--helper: `get_terms` & `bisect_terms` shell function in CPranit Bauva, Jul 31, 2016
  14. Junio C HamanoAug 2, 2016
  15. Pranit BauvaAug 3, 2016
  16. 08/13 bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in CPranit Bauva, Jul 31, 2016
  17. Junio C HamanoAug 2, 2016
  18. 09/13 bisect--helper: `bisect_write` shell function in CPranit Bauva, Jul 31, 2016
  19. Junio C HamanoAug 2, 2016
  20. Junio C HamanoAug 2, 2016
  21. Pranit BauvaAug 3, 2016
  22. Pranit BauvaAug 3, 2016
  23. 02/13 bisect: rewrite `check_term_format` shell function in CPranit Bauva, Jul 31, 2016
  24. Junio C HamanoAug 2, 2016
  25. Pranit BauvaAug 3, 2016
  26. 10/13 bisect--helper: `check_and_set_terms` shell function in CPranit Bauva, Jul 31, 2016
  27. Junio C HamanoAug 2, 2016
  28. Pranit BauvaAug 3, 2016
  29. 13/13 bisect--helper: `bisect_start` shell function partially in CPranit Bauva, Jul 31, 2016
  30. Junio C HamanoAug 2, 2016
  31. Pranit BauvaAug 3, 2016
  32. 04/13 bisect--helper: `bisect_clean_state` shell function in CPranit Bauva, Jul 31, 2016
  33. Junio C HamanoAug 2, 2016
  34. Pranit BauvaAug 3, 2016
  35. Junio C HamanoAug 4, 2016
  36. Pranit BauvaAug 4, 2016
  37. Junio C HamanoAug 4, 2016
  38. Pranit BauvaAug 4, 2016
  39. Junio C HamanoAug 2, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.