Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:

Reimplement the `bisect_state` shell function in C and also add a
subcommand `--bisect-state` to `git-bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-state` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other methods.

`bisect_head` is called from `bisect_state` thus its not required to
introduce another subcommand.

Missing comma before "thus", and "it is" (or "it's") instead of "its" :)

This is very shellish.
In C I'd expect something like

static int bisect_head_sha1(unsigned char *sha)
{
	int res;
	if (is_empty_or_missing_file(git_path_bisect_head()))
		res = get_sha1("HEAD", sha);
	else
		res = get_sha1("BISECT_HEAD", sha);

	if (res)
		return error(_("Could not find BISECT_HEAD or HEAD."));

	return 0;
}

+
+static int bisect_state(struct bisect_terms *terms, const char **argv,
+			int argc)
+{
+	const char *state = argv[0];
+
+	get_terms(terms);
+	if (check_and_set_terms(terms, state))
+		return -1;
+
+	if (!argc)
+		die(_("Please call `--bisect-state` with at least one argument"));

I think this check should move to cmd_bisect__helper. There are also the
other argument number checks.

+
+	if (argc == 1 && one_of(state, terms->term_good,
+	    terms->term_bad, "skip", NULL)) {
+		const char *bisected_head = xstrdup(bisect_head());
+		const char *hex[1];

Maybe:
		const char *hex;

+		unsigned char sha1[20];
+
+		if (get_sha1(bisected_head, sha1))
+			die(_("Bad rev input: %s"), bisected_head);

And instead of...

+		if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
+			return -1;
+
+		*hex = xstrdup(sha1_to_hex(sha1));
+		if (check_expected_revs(hex, 1))
+			return -1;

... simply:

		hex = xstrdup(sha1_to_hex(sha1));
		if (set_state(terms, state, hex)) {
			free(hex);
			return -1;
		}
		free(hex);

where:

static int set_state(struct bisect_terms *terms, const char *state,
                                                 const char *hex)
{
	if (bisect_write(state, hex, terms, 0))
		return -1;
	if (check_expected_revs(&hex, 1))
		return -1;
	return 0;
}

+		return bisect_auto_next(terms, NULL);
+	}
+
+	if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
+			one_of(state, terms->term_good, "skip", NULL)) {
+		int i;
+		struct string_list hex = STRING_LIST_INIT_DUP;
+
+		for (i = 1; i < argc; i++) {
+			unsigned char sha1[20];
+
+			if (get_sha1(argv[i], sha1)) {
+				string_list_clear(&hex, 0);
+				die(_("Bad rev input: %s"), argv[i]);
+			}
+			string_list_append(&hex, sha1_to_hex(sha1));
+		}
+		for (i = 0; i < hex.nr; i++) {

... And replace this:

+			const char **hex_string = (const char **) &hex.items[i].string;
+			if(bisect_write(state, *hex_string, terms, 0)) {
+				string_list_clear(&hex, 0);
+				return -1;
+			}
+			if (check_expected_revs(hex_string, 1)) {
+				string_list_clear(&hex, 0);
+				return -1;
+			}

by:

			const char *hex_str = hex.items[i].string;
			if (set_state(terms, state, hex_string)) {
				string_list_clear(&hex, 0);
				return -1;
			}

"mark the state of the given revs"

Note that rev != ref

"at least one revision"

[...deleted lines...]

+	eval git bisect--helper --bisect-state 'skip' $all

I think you don't need "eval" here any longer.

The new comment is funny, but you don't need a subshell here any longer.

~Stephan