Re: [PATCH 2/3] revision: small readability improvement for reading from stdin
From: Junio C Hamano <hidden>
Date: 2023-06-14 16:03:39
Patrick Steinhardt [off-list ref] 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.
quoted hunk ↗ jump to hunk
Signed-off-by: Patrick Steinhardt <redacted> --- 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);