Re: [PATCH v15 23/27] bisect--helper: `bisect_replay` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Nov 22, 2016, 00:49 UTC
Message-ID: <1816d5b4-a4c1-7c97-09ff-b11001501423@gmx.net>
In-Reply-To: <01020157c38b1b29-65f79716-42c6-4327-acda-8c8d0fe05471-000000@eu-west-1.amazonses.com>

Okay Pranit,

this is the last patch for me to review in this series.

Now that I have a coarse overview of what you did, I have the general remark that imho the "terms" variable should simply be global instead of being passed around all the time.

I also had some other remarks but I forgot them... maybe they come to my mind again when I see patch series v16.

I also want to remark again that I am not a Git developer and only reviewed this because of my interest in git-bisect. So some of my suggestions might conflict with other beliefs here. For example, I consider it very bad style to leak memory... but Git is rather written as a scripting tool than a genuine library, so perhaps many people here do not care about it as long as it works...

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

Show 9 quoted lines

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c18ca07..b367d8d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -601,7 +602,6 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>  			terms->term_good = arg;
>  		} else if (!strcmp(arg, "--term-bad") ||
>  			 !strcmp(arg, "--term-new")) {
> -			const char *next_arg;

This should already have been removed in patch 15/27, not here.

Show 25 quoted lines

> @@ -875,6 +875,117 @@ static int bisect_log(void)
>  	return status;
>  }
>  
> +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;
> +}
> +
> +static int bisect_replay(struct bisect_terms *terms, const char *filename)
> +{
> +	struct strbuf line = STRBUF_INIT;
> +	struct strbuf word = STRBUF_INIT;
> +	FILE *fp = NULL;

(The initialization is not necessary here.)

> +	int res = 0;
> +
> +	if (is_empty_or_missing_file(filename)) {
> +		error(_("no such file with name '%s' exists"), filename);

The error message is misleading if the file exists but is empty. Maybe something like "cannot read file '%s' for replaying"?

> +		res = -1;
> +		goto finish;

		goto fail;
:D

Show 5 quoted lines

> +	}
> +
> +	if (bisect_reset(NULL)) {
> +		res = -1;
> +		goto finish;

		goto fail;

Show 6 quoted lines

> +	}
> +
> +	fp = fopen(filename, "r");
> +	if (!fp) {
> +		res = -1;
> +		goto finish;

		goto fail;

Show 15 quoted lines

> +	}
> +
> +	while (strbuf_getline(&line, fp) != EOF) {
> +		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 (!strcmp(word.buf, "#")) {
> +				break;

Maybe it is more robust to check whether word.buf begins with #

Show 6 quoted lines

> +			}
> +
> +			get_terms(terms);
> +			if (check_and_set_terms(terms, word.buf)) {
> +				res = -1;
> +				goto finish;

				goto fail;

Show 9 quoted lines

> +			}
> +
> +			if (!strcmp(word.buf, "start")) {
> +				struct argv_array argv = ARGV_ARRAY_INIT;
> +				sq_dequote_to_argv_array(line.buf+pos, &argv);
> +				if (bisect_start(terms, 0, argv.argv, argv.argc)) {
> +					argv_array_clear(&argv);
> +					res = -1;
> +					goto finish;

					goto fail;

Show 10 quoted lines

> +				}
> +				argv_array_clear(&argv);
> +				break;
> +			}
> +
> +			if (one_of(word.buf, terms->term_good,
> +			    terms->term_bad, "skip", NULL)) {
> +				if (bisect_write(word.buf, line.buf+pos, terms, 0)) {
> +					res = -1;
> +					goto finish;

					goto fail;

Show 11 quoted lines

> +				}
> +				break;
> +			}
> +
> +			if (!strcmp(word.buf, "terms")) {
> +				struct argv_array argv = ARGV_ARRAY_INIT;
> +				sq_dequote_to_argv_array(line.buf+pos, &argv);
> +				if (bisect_terms(terms, argv.argv, argv.argc)) {
> +					argv_array_clear(&argv);
> +					res = -1;
> +					goto finish;

					goto fail;

Show 6 quoted lines

> +				}
> +				argv_array_clear(&argv);
> +				break;
> +			}
> +
> +			error(_("?? what are you talking about?"));

I know this string is taken from the original source. However, even something like error(_("Replay file contains rubbish (\"%s\")"), word.buf) is more informative.

> +			res = -1;
> +			goto finish;

			goto fail;

> +		}
> +	}
> +	goto finish;

+fail: + res = -1;

I just wanted to make finally clear what I was referring to by the "goto fail" trick. :D

Also I think the readability could be improved by extracting the body of the outer while loop into an extra function replay_line(). Then most of my suggested "goto fail;" lines simply become "return -1;" :)

Show 7 quoted lines

> @@ -998,6 +1112,13 @@ int cmd_bisect__helper(...)
>   			die(_("--bisect-log requires 0 arguments"));
>   		res = bisect_log();
>   		break;
> +	case BISECT_REPLAY:
> +		if (argc != 1)
> +			die(_("--bisect-replay requires 1 argument"));

I'd keep the (already translated) string from the original source: "No logfile given"

Cheers,
  Stephan