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

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

From
Rafael Silva <rafaeloliveira.cs@gmail.com>
Date
Dec 22, 2020, 00:15 UTC
Message-ID
<gohp6ky2hqzovu.fsf@cpm12071.fritz.box>
In-Reply-To
<20201221162743.96056-3-mirucam@gmail.com>
Miriam Rubio writes:
Show 53 quoted lines
> 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 ;).

Show 24 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;
> +		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.

Show 5 quoted lines
> +			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)
Show 13 quoted lines
> +			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.

Show 7 quoted lines
> +			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
Show 22 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;
> +}
> +
-- 
Thanks
Rafael
Previous: Miriam RubioNext: Rafael Silva
Message 11 of 18 in “Finish converting git bisect to C part 3”
  1. 0/7 Finish converting git bisect to C part 3Miriam Rubio, Dec 21, 2020
  2. 5/7 bisect--helper: retire `--bisect-auto-next` subcommandMiriam Rubio, Dec 21, 2020
  3. 6/7 bisect--helper: reimplement `bisect_skip` shell function in CMiriam Rubio, Dec 21, 2020
  4. Johannes SchindelinJan 18, 2021
  5. 1/7 bisect--helper: reimplement `bisect_log` shell function in CMiriam Rubio, Dec 21, 2020
  6. Johannes SchindelinJan 18, 2021
  7. 3/7 bisect--helper: retire `--bisect-write` subcommandMiriam Rubio, Dec 21, 2020
  8. 7/7 bisect--helper: retire `--check-and-set-terms` subcommandMiriam Rubio, Dec 21, 2020
  9. 4/7 bisect--helper: use `res` instead of return in BISECT_RESET case optionMiriam Rubio, Dec 21, 2020
  10. 2/7 bisect--helper: reimplement `bisect_replay` shell function in CMiriam Rubio, Dec 21, 2020
  11. Rafael SilvaDec 22, 2020
  12. Rafael SilvaDec 23, 2020
  13. Junio C HamanoDec 23, 2020
  14. Rafael SilvaDec 23, 2020
  15. Junio C HamanoDec 23, 2020
  16. Johannes SchindelinJan 18, 2021
  17. Johannes SchindelinJan 18, 2021
  18. Junio C HamanoJan 18, 2021

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.