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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2016, 23:24 UTC
Message-ID: <xmqq7fb3dsg9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <01020156b73fe6d7-6cd42223-f46c-4e4c-a15f-516fc13a4521-000000@eu-west-1.amazonses.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 8 quoted lines

> +static int get_next_word(struct strbuf *line, struct strbuf *word)
> +{
> +	int i;
> +	for (i = 0; line->buf[i] != ' ' && line->buf[i] != '\0'; i++)
> +		strbuf_addch(word, line->buf[i]);
> +
> +	return 0;
> +}

This looks like a very non-standard way to use a string buffer. The function does not even have to take line as a whole strbuf, because the logic does not even look at line->len and relies on line->buf[] to be NUL terminated, which means the first parameter should instead be "const char *buf", and the caller would feed then line.buf to the function.

And I highly suspect that this is a very suboptimal interface, if the caller then has to compute up to which byte in the line.buf to splice away to get to the "next word" again.

A better alternative with higher level of abstraction would instead be to keep the two strbuf parameters as-is, but have this function responsible for "moving" the first word of "line" strbuf into "word" strbuf. Then the caller can repeatedly call this function to get each word in "word" strbuf until "line" becomes empty if it wants to implement "a word at a time" operation.

If that higher level of abstraction is too limiting for the caller (and also that would be just as inefficient as the patch under discussion), another alternative would be to have the caller maintain "the current byte position in the input" and do something like:

	int pos = 0;

	while (pos < line.len) {
                strbuf_reset(&word);
                get_next_word(line.buf, pos, word);
                pos += word.len;
		do a thing on "word";
	}

to implement "a word at a time" operation. For this to work, get_next_word() would need to skip the leading blanks itself, of course.

In any case, I won't comment on the body of the function too deeply; it will probably become a lot cleaner if you employed the "retval + goto finish:" pattern for error handling. Use of dequote seems correct from the cursory look, too.