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

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

Miriam Rubio writes:

> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Reimplement the `bisect_replay` shell function in C and also add
> `--bisect-replay` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
>
> Using `--bisect-replay` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  builtin/bisect--helper.c | 127 ++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            |  34 +----------
>  2 files changed, 127 insertions(+), 34 deletions(-)
>
> 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
> @@ -31,6 +31,7 @@ static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --bisect-auto-next"),
>  	N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
>  	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> +	N_("git bisect--helper --bisect-replay <filename>"),
>  	NULL
>  };
>  
> @@ -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;
> +}
> +

I would like to suggest a slight different approach to handling the "is
the begin of the loop?" logic. If I understood correctly, the `begin`
variable is to check whether is the beginning of the word processing
and is changed to 1 (aka: to true) on the first loop interaction after
the loop is executed for the first time.

However, I believe we can check this information in different way that
will simplify the logic by removing the "begin" and the
"if (!begin)..." altogether. The for-loop is initialize with "i" set to
"pos" which means that on the first execution the expression "i == pos"
is going to be true, and "false" for the next interactions. Thus, checking
if "i" is different, or better checking if "i" is greater should bring
the same result.

that said, the implementation might look like this:

  	strbuf_reset(word);
	for (i = pos; i < len; i++) {
		if (line[i] == ' ' && i > pos)
			return i + 1;

		strbuf_addch(word, line[i]);
	}

With additionally, removing the "begin" from the beginning of the function.

The above was copied into the email without major tests, although
I have this implementation locally just to ensure its compiles
successfully.

Please take this suggestion as you wish, I do not have any strong
opinion on the current implementation. Also, I'm a recent contributor to
the project and should not be much trusted when proposing changes as when
proposal comes from project maintainer and/or Senior contributors ;).

> +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;
> +		else if (starts_with(word->buf, "#"))
> +			break;
> +
> +		get_terms(terms);
> +		if (check_and_set_terms(terms, word->buf))
> +			return -1;
> +
> +		if (!strcmp(word->buf, "start")) {
> +			struct strvec argv = STRVEC_INIT;
> +			int res;

I believe this variable is already defined and initialize on the
beginning of the function, right? If that is the case then the
declaration seems duplicated here and can be avoided.

> +			sq_dequote_to_strvec(line->buf+pos, &argv);
> +			res = bisect_start(terms, argv.v, argv.nr);
> +			strvec_clear(&argv);
> +			if (res)
> +				return -1;

Also, (see bellow)

> +			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;
> +			break;
> +		}
> +
> +		if (!strcmp(word->buf, "terms")) {
> +			struct strvec argv = STRVEC_INIT;
> +			int res;

In case this supposed to be the same from the beginning of the function,
the declaration seems duplicated here as well. 

> +			sq_dequote_to_strvec(line->buf+pos, &argv);
> +			res = bisect_terms(terms, argv.nr == 1 ? argv.v[0] : NULL);
> +			strvec_clear(&argv);
> +			if (res)
> +				return -1;
> +			break;
> +		}

Another suggestion, again take as you wish, you can place the "if"
directly on the call of the "bisect_start()" and set the "res = -1"
as the value of "res" will be used for the function return anyways.
Again with the intent of simplify the implementation.

The code will look something like the similar:

        if (bisect_terms(terms, argv.nr == 1 ? argv.v[0] : NULL))
                res = -1;
        strvec_clear(&argv);
        break;

I did not test the above code thoroughly though.


> +		error(_("Replay file contains rubbish (\"%s\")"),
> +		      word->buf);

I think this will be nicer on the same line ;). Not worth a re-roll

> +		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;
> +}
> +


-- 
Thanks
Rafael