Re: [PATCH v2 2/7] bisect--helper: reimplement `bisect_replay` shell function in C

Hi Miriam,

this patch looks pretty good to me. I just have a couple comments/suggestions:

On Mon, 21 Dec 2020, Miriam Rubio wrote:

While I have different objections than Rafael (the function seems to want to left-trim, but does an inadequate job at it), I do not even think that we need this function. See below.

This is not quite correct, as it would skip arbitrary amounts of "git" and "git-bisect" and "bisect", even in the middle of the line.

Besides, this `while()` loop iterates over the characters of the current line, while the original loop iterated over the _lines_:

	while read git bisect command rev
	do
		test "$git $bisect" = "git bisect" || test "$git" = "git-bisect" || continue
	[...]

As you can see, lines that do not start with "git bisect" or "git-bisect" are simply ignored by `continue`ing to the next line. In the C code, `continue` would continue to the next _word_.

I'd like to strongly suggest a very different approach, where no `while()` loop is used in `process_line()` (BTW can we please rename this to `process_replay_line()` to 1) make it less generic a name and 2) convey via the name what this function is about?):

	const char *p;

	if (!skip_prefix(line->buf, "git bisect", &p) &&
	    !skip_prefix(line->buf, "git bisect", p))
		return 0;

As the original code used `read git bisect command rev` to parse the line, which automatically trims white-space, we might want to do something similar to that:

	const char *p = line->buf + strspn(line->buf, " \t");

	if ((!skip_prefix(p, "git bisect", &p) &&
	     !skip_prefix(p, "git-bisect", &p)) ||
	    !isspace(*p))
		return 0;
	p += strspn(p, " \t");

Please note that the original shell code is _a lot_ more forgiving: it ignores _anything_ but "git bisect" and "git-bisect" at the beginning of the (white-space-trimmed) line. Not just comments starting with `#`. I'd like to return to the shell script's behavior.

Do we really have to read the terms for every line we replay? I guess we do, as every time we use new/old after good/bad (or vice versa), the terms file gets rewritten.

We might want to address this awkwardness in the future: in C, we do not have to read and write the terms file _all_ the time.

Let's use `skip_prefix()` here, too:

		char *word_end = p + strcspn(p, " \t");
		const char *rev = word_end + strspn(word_end, " \t");

		*word_end = '\0'; /* NUL-terminate the word */

		if (!strcmp("start", p)) {
			struct strvec argv = STRVEC_INIT;
			int res;

			sq_dequote_to_strvec(rev, &argv);
			res = bisect_start(terms, argv.v, argv.nr);
			strvec_clear(&argv);
			return res;
		}

Apart from using `p` as above instead of `word->buf`, and `rev` instead of `line->buf+pos`, why not returning directly what `bisect_write()` returned?

		if (one_of(p, terms->term_good, terms->term_bad, "skip", NULL))
			return bisect_write(p, rev, terms, 0);

We should probably error out if `argv.nr > 1`.

Let's `return res` directly.

The shell script version does this instead:

		die "$(gettext "?? what are you talking about?")" ;;

We should do the same, if only to make life easier on the translators.

A lot of this function is just boiler plate. I'd prefer to merge the code into `bisect_replay()` instead.

The rest looks good to me!

Ciao, Dscho