Re: [PATCH 2/3] revision: small readability improvement for reading from stdin

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> The code that reads lines from standard input manually compares whether
> the read line matches "--", which is a bit awkward to read. Refactor it
> to instead use strcmp(3P) for a small code style improvement.

It is unclear if it is an "improvement".  We've already checked the
first byte to be "-" and we only need to check the second one (and
the length of the string) to see if we have a double-dash, instead
of checking the first byte again.

I am not sure if these excessive blank lines are helping, either.

The only reason I would be inclined to support the main change in
this patch (but not additional blank lines) is because I will be
suggesting to lift the logic to detect double-dash from the "line
begins with dash" block in my review of the next step.  Once that is
done, double-dash detection cannot rely on the fact that we have
already checked the first byte.

I do agree with the change to remove the temporary variable "len",
if we were to remove the "len == 2" comparison, as it makes "len"
less useful.

Thanks.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  revision.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index cc22ccd76e..dcb7951b4e 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2795,16 +2795,18 @@ static void read_revisions_from_stdin(struct rev_info *revs,
>  
>  	strbuf_init(&sb, 1000);
>  	while (strbuf_getline(&sb, stdin) != EOF) {
> -		int len = sb.len;
> -		if (!len)
> +		if (!sb.len)
>  			break;
> +
>  		if (sb.buf[0] == '-') {
> -			if (len == 2 && sb.buf[1] == '-') {
> +			if (!strcmp(sb.buf, "--")) {
>  				seen_dashdash = 1;
>  				break;
>  			}
> +
>  			die("options not supported in --stdin mode");
>  		}
> +
>  		if (handle_revision_arg(sb.buf, revs, 0,
>  					REVARG_CANNOT_BE_FILENAME))
>  			die("bad revision '%s'", sb.buf);