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

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

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 25, 2016, 19:02 UTC
Message-ID
<xmqqd1kwn035.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<01020156b73fe6b9-5e500c96-9bc3-4a6b-beb9-b5988dc9ee85-000000@eu-west-1.amazonses.com>
Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 7 quoted lines
> +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;
> +	struct string_list revs = STRING_LIST_INIT_DUP;
> +	struct string_list states = STRING_LIST_INIT_DUP;

The original has a single state, not states. Let's see if there is a reason behind the name change....

> +	unsigned char sha1[20];
> +	struct object_id oid;
More on these below...
Show 23 quoted lines
> + ...
> +	for (i = 0; i < argc; i++) {
> +		const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> +		if (!strcmp(argv[i], "--")) {
> +			has_double_dash = 1;
> +			break;
> +		} else if (!strcmp(argv[i], "--no-checkout"))
> +			no_checkout = 1;
> +		else if (!strcmp(argv[i], "--term-good") ||
> + ...
> +		} else 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]);
> +		} else if (get_oid(commit_id, &oid) && has_double_dash) {
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			die(_("'%s' does not appear to be a valid revision"), argv[i]);
> +		} else {
> +			string_list_append(&revs, oid_to_hex(&oid));
> +		}
> +	}

What I do not understand is clearing the string list "states" inside this loop. It may have been INIT_DUPed, but nothing in this loop adds anything to it. Because "revs" does get extended in the loop, it is understandable if you wanted to clear it before dying, but "if you are dying anyway why bother clearing?" is also a valid stance to take.

The same "perhaps want to use the 'retval' with goto 'finish:' pattern?" comment applies here, too.

Show 17 quoted lines
> +	pathspec_pos = i;
> +
> +	/*
> +	 * 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 |= !!revs.nr;
> +	for (i = 0; i < revs.nr; i++) {
> +		if (bad_seen)
> +			string_list_append(&states, terms->term_good.buf);
> +		else {
> +			bad_seen = 1;
> +			string_list_append(&states, terms->term_bad.buf);
> +		}
> +	}

This is certainly different from the original. We used to do one "bisect_write" per element in revs in the loop; we no longer do that and instead collect them in states list. Each element in these two lists, i.e. revs.item[i] and states.item[i], corresponds to each other.

That explains why the variable is renamed to states. We haven't seen enough to say if this behaviour change is a good idea or not.

Show 27 quoted lines
> +	/*
> +	 * Verify HEAD
> +	 */
> +	head = resolve_ref_unsafe("HEAD", 0, sha1, &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"));
> +		}
> +	}
> +	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 'git "
> +					"bisect start <valid-branch>'."),
> +				      start_head.buf);
> +				strbuf_release(&start_head);
> +				string_list_clear(&revs, 0);
> +				string_list_clear(&states, 0);
> +				return -1;

The original died here, but you expect the caller to respond to a negative return. I haven't read enough to judge if that is a good idea, but doesn't it make sense to do the same throughout the function consistently? I saw a few die()'s already in the command line parsing loop--shouldn't they also return -1?

The original has called bisect_write already when we attempt this checkout and the user will see the states in the filesystem. This rewrite does not. What effect does this behaviour change have to the end-user experience? The error message tells the end user to run another "git bisect start" with a valid commit, and when that happens, hopefully she will give us something we can check out, and then we will hit the bisect_clean_state() call we see below before starting to do anything meaningful, so I am guessing it won't, but I just want to make sure that you thought about the ramifications of the change above to delay calls to bisect_write.

Show 5 quoted lines
> +			}
> +		}
> +	} else {
> +		if (starts_with(head, "refs/heads/") ||
> +		    !get_oid_hex(head, &oid) || ref_exists(head)) {

This ref_exists() check is new, and I think it should not be there. In the original, if .git/HEAD pointed at refs/tags/v1.0, we would have diagnosed it as a strange symbolic ref. This no longer does.

If you wanted to make sure that the branch exists, it should have been

	if ((starts_with(head, "refs/heads/") && ref_exists(head)) ||
	    !get_oid_hex(head, &oid)) {
anyway.

Also, why do you use get_oid_hex() here? You used get_sha1("HEAD", sha1) in the early part of "Verify HEAD" above, which seems to be perfectly adequate. That sha1 taken from get_sha1() is what you end up using in the code below anyway. If you can stick to one or the other, please do so.

Show 11 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"));
> +			}

Not to be done as part of this series, but it probably is safe to retire this part by now. Cogito has been dead for how many years?

> +			if (starts_with(head, "refs/heads/")) {
> +				strbuf_reset(&start_head);
> +				strbuf_addstr(&start_head, head + 11);
skip_prefix?
Show 12 quoted lines
> +			}
> +			else {
> +				strbuf_reset(&start_head);
> +				strbuf_addstr(&start_head, sha1_to_hex(sha1));
> +			}
> +		} else {
> +			strbuf_release(&start_head);
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			die(_("Bad HEAD - strange symbolic ref"));
> +		}
> +	}

I wonder the whole thing above is better restructured to avoid repeated checks of the same thing.

	if (is it 40-hex, i.e. detached?) {
		stuff it to start_head;
	} else if (skip_prefix(head, "refs/heads/", &branchname)) {
        	do the "cogito" check;
                stuff it to start_head;
	} else {
        	that's a strange symbolic ref HEAD you have there;
	}
Show 6 quoted lines
> +	/*
> +	 * Get rid of any old bisect state.
> +	 */
> +	if (bisect_clean_state()) {
> +		return -1;
> +	}

If we are going to get a lot more code inside {} in later patches, then this single "return -1" enclosed inside a {} pair is justifiable (but in that case we'd prefer an empty line after it to separate it from the next comment block). Otherwise lose the {}.

Show 6 quoted lines
> +	/*
> +	 * In case of mistaken revs or checkout error, or signals received,
> +	 * "bisect_auto_next" below may exit or misbehave.
> +	 * We have to trap this to be able to clean up using
> +	 * "bisect_clean_state".
> +	 */

That explains the "trap" statements in the original. Does it apply to this code here?

Show 30 quoted lines
> +	/*
> +	 * Write new start state
> +	 */
> +	write_file(git_path_bisect_start(), "%s\n", start_head.buf);
> +
> +	if (no_checkout) {
> +		get_oid(start_head.buf, &oid);
> +		if (update_ref(NULL, "BISECT_HEAD", oid.hash, NULL, 0,
> +			       UPDATE_REFS_MSG_ON_ERR)) {
> +			strbuf_release(&start_head);
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			return -1;
> +		}
> +	}
> +	strbuf_release(&start_head);
> +
> +	if (pathspec_pos < argc - 1)
> +		sq_quote_argv(&bisect_names, argv + pathspec_pos, 0);
> +	write_file(git_path_bisect_names(), "%s\n", bisect_names.buf);
> +	strbuf_release(&bisect_names);
> +
> +	for (i = 0; i < states.nr; i++) {
> +		if (bisect_write(states.items[i].string,
> +				  revs.items[i].string, terms, 1)) {
> +			string_list_clear(&revs, 0);
> +			string_list_clear(&states, 0);
> +			return -1;
> +		}
> +	}

Hmph. I do not particuarly see why doing this in a separate loop here, instead of doing it just like in the original, i.e. inside the loop we already saw, is an improvement. It seems to me that the only effect of this change is to make the code more complex by forcing you to maintain (and clear) another string list "states" and have a separate loop here.

Unless there is a reason why delaying calls to bisect_write() is a good thing and I am not seeing it, that is.

Show 12 quoted lines
> diff --git a/git-bisect.sh b/git-bisect.sh
> index d6c8b5a..f0896b3 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -71,122 +71,7 @@ bisect_autostart() {
>  }
>  
>  bisect_start() {
> -...
> +	git bisect--helper --bisect-start $@ || exit
>  ...
> +	get_terms

Understandable. As the handling of the terms is done in the helper, you need to read the terms it left in the filesystem. OK.

>  	bisect_auto_next
Previous: Pranit BauvaNext: Junio C Hamano
Message 4 of 62 in “bisect--helper: use OPT_CMDMODE instead of OPT_BOOL”
  1. 01/27 bisect--helper: use OPT_CMDMODE instead of OPT_BOOLPranit Bauva, Aug 23, 2016
  2. 25/27 bisect--helper: retire `--bisect-autostart` subcommandPranit Bauva, Aug 23, 2016
  3. 13/27 bisect--helper: `bisect_start` shell function partially in CPranit Bauva, Aug 23, 2016
  4. Junio C HamanoAug 25, 2016
  5. Junio C HamanoAug 25, 2016
  6. Pranit BauvaAug 27, 2016
  7. Junio C HamanoAug 27, 2016
  8. 14/27 bisect--helper: `bisect_next` and `bisect_auto_next` shell function in CPranit Bauva, Aug 23, 2016
  9. Junio C HamanoAug 25, 2016
  10. Pranit BauvaAug 30, 2016
  11. Pranit BauvaAug 30, 2016
  12. Junio C HamanoAug 30, 2016
  13. Pranit BauvaAug 30, 2016
  14. 16/27 bisect--helper: retire `--next-all` subcommandPranit Bauva, Aug 23, 2016
  15. 06/27 wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()Pranit Bauva, Aug 23, 2016
  16. 27/27 bisect--helper: remove the dequote in bisect_start()Pranit Bauva, Aug 23, 2016
  17. 05/27 t6030: explicitly test for bisection cleanupPranit Bauva, Aug 23, 2016
  18. 19/27 bisect--helper: retire `--check-expected-revs` subcommandPranit Bauva, Aug 23, 2016
  19. 07/27 bisect--helper: `bisect_reset` shell function in CPranit Bauva, Aug 23, 2016
  20. Junio C HamanoAug 24, 2016
  21. Pranit BauvaAug 26, 2016
  22. Junio C HamanoAug 26, 2016
  23. Pranit BauvaAug 27, 2016
  24. Junio C HamanoAug 29, 2016
  25. 15/27 bisect--helper: retire `--bisect-clean-state` subcommandPranit Bauva, Aug 23, 2016
  26. Junio C HamanoAug 26, 2016
  27. 09/27 bisect--helper: `bisect_write` shell function in CPranit Bauva, Aug 23, 2016
  28. Junio C HamanoAug 24, 2016
  29. Pranit BauvaAug 27, 2016
  30. Junio C HamanoAug 27, 2016
  31. Pranit BauvaAug 30, 2016
  32. 18/27 bisect--helper: `bisect_state` & `bisect_head` shell function in CPranit Bauva, Aug 23, 2016
  33. 21/27 bisect--helper: `bisect_log` shell function in CPranit Bauva, Aug 23, 2016
  34. Junio C HamanoAug 26, 2016
  35. Pranit BauvaAug 27, 2016
  36. 24/27 bisect--helper: retire `--check-and-set-terms` subcommandPranit Bauva, Aug 23, 2016
  37. 17/27 bisect--helper: `bisect_autostart` shell function in CPranit Bauva, Aug 23, 2016
  38. Junio C HamanoAug 26, 2016
  39. 12/27 bisect--helper: `get_terms` & `bisect_terms` shell function in CPranit Bauva, Aug 23, 2016
  40. Junio C HamanoAug 25, 2016
  41. Pranit BauvaAug 27, 2016
  42. Junio C HamanoAug 29, 2016
  43. 04/27 bisect--helper: `bisect_clean_state` shell function in CPranit Bauva, Aug 23, 2016
  44. Junio C HamanoAug 24, 2016
  45. 20/27 bisect--helper: retire `--write-terms` subcommandPranit Bauva, Aug 23, 2016
  46. 23/27 bisect--helper: retire `--bisect-write` subcommandPranit Bauva, Aug 23, 2016
  47. 10/27 bisect--helper: `check_and_set_terms` shell function in CPranit Bauva, Aug 23, 2016
  48. 03/27 bisect--helper: `write_terms` shell function in CPranit Bauva, Aug 23, 2016
  49. 26/27 bisect--helper: retire `--bisect-auto-next` subcommandPranit Bauva, Aug 23, 2016
  50. 02/27 bisect: rewrite `check_term_format` shell function in CPranit Bauva, Aug 23, 2016
  51. 08/27 bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in CPranit Bauva, Aug 23, 2016
  52. Junio C HamanoAug 24, 2016
  53. Pranit BauvaAug 27, 2016
  54. Junio C HamanoAug 29, 2016
  55. 22/27 bisect--helper: `bisect_replay` shell function in CPranit Bauva, Aug 23, 2016
  56. Junio C HamanoAug 26, 2016
  57. 11/27 bisect--helper: `bisect_next_check` & bisect_voc shell function in CPranit Bauva, Aug 23, 2016
  58. Junio C HamanoAug 24, 2016
  59. Pranit BauvaAug 27, 2016
  60. Pranit BauvaAug 23, 2016
  61. Junio C HamanoAug 23, 2016
  62. Pranit BauvaAug 23, 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.