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:
Show 24 quoted lines
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1854377fa6..92c783237d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -916,6 +917,121 @@ static enum bisect_error bisect_log(void)
> return status ? BISECT_FAILED : BISECT_OK;
> }
>
> +static int get_next_word(const char *line, int pos, struct strbuf *word)
> +{
> + int i, len = strlen(line), begin = 0;
> +
> + strbuf_reset(word);
> + for (i = pos; i < len; i++) {
> + if (line[i] == ' ' && begin)
> + return i + 1;
> +
> + if (!begin)
> + begin = 1;
> + strbuf_addch(word, line[i]);
> + }
> +
> + return i;
> +}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.
Show 15 quoted lines
> +
> +static int process_line(struct bisect_terms *terms, struct strbuf *line, struct strbuf *word)
> +{
> + int res = 0;
> + int pos = 0;
> +
> + while (pos < line->len) {
> + pos = get_next_word(line->buf, pos, word);
> +
> + if (!strcmp(word->buf, "git"))
> + continue;
> + else if (!strcmp(word->buf, "git-bisect"))
> + continue;
> + else if (!strcmp(word->buf, "bisect"))
> + continue;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");
> + else if (starts_with(word->buf, "#"))
> + break;
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.
> +
> + get_terms(terms);
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.
> + if (check_and_set_terms(terms, word->buf))
> + return -1;
> +
> + if (!strcmp(word->buf, "start")) {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;
}
Show 14 quoted lines
> + struct strvec argv = STRVEC_INIT;
> + int res;
> + sq_dequote_to_strvec(line->buf+pos, &argv);
> + res = bisect_start(terms, argv.v, argv.nr);
> + strvec_clear(&argv);
> + if (res)
> + return -1;
> + break;
> + }
> +
> + if (one_of(word->buf, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + if (bisect_write(word->buf, line->buf+pos, terms, 0))
> + return -1;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);
> + break;
> + }
Show 6 quoted lines
> +
> + if (!strcmp(word->buf, "terms")) {
> + struct strvec argv = STRVEC_INIT;
> + int res;
> + sq_dequote_to_strvec(line->buf+pos, &argv);
> + res = bisect_terms(terms, argv.nr == 1 ? argv.v[0] : NULL);We should probably error out if `argv.nr > 1`.
> + strvec_clear(&argv);
> + if (res)
> + return -1;
> + break;
Let's `return res` directly.
> + }
> +
> + error(_("Replay file contains rubbish (\"%s\")"),
> + word->buf);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.
Show 21 quoted lines
> + res = -1;
> + }
> + return res;
> +}
> +
> +static int process_replay_file(FILE *fp, struct bisect_terms *terms)
> +{
> + struct strbuf line = STRBUF_INIT;
> + struct strbuf word = STRBUF_INIT;
> + int res = 0;
> +
> + while (strbuf_getline(&line, fp) != EOF) {
> + res = process_line(terms, &line, &word);
> + if (res)
> + break;
> + }
> +
> + strbuf_release(&line);
> + strbuf_release(&word);
> + return res;
> +}A lot of this function is just boiler plate. I'd prefer to merge the code into `bisect_replay()` instead.
Show 24 quoted lines
> +
> +static enum bisect_error bisect_replay(struct bisect_terms *terms, const char *filename)
> +{
> + FILE *fp = NULL;
> + enum bisect_error res = BISECT_OK;
> +
> + if (is_empty_or_missing_file(filename))
> + return error(_("cannot read file '%s' for replaying"), filename);
> +
> + if (bisect_reset(NULL))
> + return BISECT_FAILED;
> +
> + fp = fopen(filename, "r");
> + if (!fp)
> + return BISECT_FAILED;
> +
> + res = process_replay_file(fp, terms);
> + fclose(fp);
> +
> + if (res)
> + return BISECT_FAILED;
> +
> + return bisect_auto_next(terms, NULL);
> +}The rest looks good to me!
Ciao, Dscho
Miriam Rubio
