Re: [PATCH v4 05/21] check-attr: there are only two possible line terminations
- From
Jeff King <peff@peff.net>
- Date
- Jan 15, 2016, 19:36 UTC
- Message-ID
- <20160115193640.GA19291@sigill.intra.peff.net>
- In-Reply-To
- <20160115191611.GB11301@sigill.intra.peff.net>
On Fri, Jan 15, 2016 at 02:16:11PM -0500, Jeff King wrote:
Show 13 quoted lines
> Not really relevant to your series, but I find it slightly confusing > when `strbuf_init` is used instead of a static initializer (it makes me > wonder _why_, and whether there are any code paths that miss > initialization). > > Maybe worth doing: > > struct strbuf buf = STRBUF_INIT; > struct strbuf nbuf = STRBUF_INIT; > > at the top while we are in the area (and also maybe giving the latter a > more meaningful name!). But I don't want to derail the main point of > your series.
Apparently we use this pattern in quite a few places (or they all copied from each other). Here's a possible fixup that could go after your series (it could go before, too, but it creates a lot of textual conflicts with your stuff).
-- >8 -- Subject: [PATCH] give "nbuf" strbuf a more meaningful name
It's a common pattern in our code to read paths from stdin, separated either by newlines or NULs, and unquote as necessary. In each of these five cases we use "nbuf" to temporarily store the unquoted value. Let's give it the more meaningful name "unquoted", which makes it easier to understand the purpose of the variable.
While we're at it, let's also static-initialize all of our strbufs. It's not wrong to call strbuf_init, but it increases the cognitive load on the reader (who might wonder "do we sometimes avoid initializing them? why?").
Signed-off-by: Jeff King <peff@peff.net> --- It's a shame that we can't just factor out this common code, but I don't think it's quite long enough to merit the boilerplate. The interesting part of each function happens inside the loop. If C had lambdas, we could do something like:
foreach_path_from(stdin, nul_term_line) {
/* now do something interesting with "buf"
and some other local variables */
}but using function pointers for this kind of iteration is pretty awful (define the two-line loop body as a separate function, stuff any local variables into a struct and pass it as "void *", etc). You can overcome that with macros and make the above code work if you don't mind creating an undebuggable mess. :)
builtin/check-attr.c | 13 ++++++------- builtin/check-ignore.c | 13 ++++++------- builtin/checkout-index.c | 11 ++++++----- builtin/hash-object.c | 11 ++++++----- builtin/update-index.c | 11 ++++++----- 5 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/builtin/check-attr.c b/builtin/check-attr.c index 087325e..53a5a18 100644 --- a/builtin/check-attr.c +++ b/builtin/check-attr.c @@ -72,24 +72,23 @@ static void check_attr(const char *prefix, int cnt, static void check_attr_stdin_paths(const char *prefix, int cnt, struct git_attr_check *check) { - struct strbuf buf, nbuf; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; strbuf_getline_fn getline_fn; getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; - strbuf_init(&buf, 0); - strbuf_init(&nbuf, 0); while (getline_fn(&buf, stdin) != EOF) { if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } check_attr(prefix, cnt, check, buf.buf); maybe_flush_or_die(stdout, "attribute to stdout"); } strbuf_release(&buf); - strbuf_release(&nbuf); + strbuf_release(&unquoted); } static NORETURN void error_with_usage(const char *msg) diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index 4f0b09e..1d73d3c 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -115,20 +115,19 @@ static int check_ignore(struct dir_struct *dir, static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix) { - struct strbuf buf, nbuf; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; char *pathspec[2] = { NULL, NULL }; strbuf_getline_fn getline_fn; int num_ignored = 0; getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; - strbuf_init(&buf, 0); - strbuf_init(&nbuf, 0); while (getline_fn(&buf, stdin) != EOF) { if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } pathspec[0] = buf.buf; num_ignored += check_ignore(dir, prefix, @@ -136,7 +135,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix) maybe_flush_or_die(stdout, "check-ignore to stdout"); } strbuf_release(&buf); - strbuf_release(&nbuf); + strbuf_release(&unquoted); return num_ignored; } diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index ed888a5..d8d7bd3 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -251,7 +251,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) } if (read_from_stdin) { - struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; strbuf_getline_fn getline_fn; if (all) @@ -261,16 +262,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) while (getline_fn(&buf, stdin) != EOF) { char *p; if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } p = prefix_path(prefix, prefix_length, buf.buf); checkout_file(p, prefix); free(p); } - strbuf_release(&nbuf); + strbuf_release(&unquoted); strbuf_release(&buf); } diff --git a/builtin/hash-object.c b/builtin/hash-object.c index ff20395..f7d3567 100644 --- a/builtin/hash-object.c +++ b/builtin/hash-object.c @@ -58,20 +58,21 @@ static void hash_object(const char *path, const char *type, const char *vpath, static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, int literally) { - struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; while (strbuf_getline(&buf, stdin) != EOF) { if (buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } hash_object(buf.buf, type, no_filters ? NULL : buf.buf, flags, literally); } strbuf_release(&buf); - strbuf_release(&nbuf); + strbuf_release(&unquoted); } int cmd_hash_object(int argc, const char **argv, const char *prefix) diff --git a/builtin/update-index.c b/builtin/update-index.c index 7c5c143..f052faf 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -1075,16 +1075,17 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) } if (read_from_stdin) { - struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT; + struct strbuf buf = STRBUF_INIT; + struct strbuf unquoted = STRBUF_INIT; setup_work_tree(); while (getline_fn(&buf, stdin) != EOF) { char *p; if (!nul_term_line && buf.buf[0] == '"') { - strbuf_reset(&nbuf); - if (unquote_c_style(&nbuf, buf.buf, NULL)) + strbuf_reset(&unquoted); + if (unquote_c_style(&unquoted, buf.buf, NULL)) die("line is badly quoted"); - strbuf_swap(&buf, &nbuf); + strbuf_swap(&buf, &unquoted); } p = prefix_path(prefix, prefix_length, buf.buf); update_one(p); @@ -1092,7 +1093,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) chmod_path(set_executable_bit, p); free(p); } - strbuf_release(&nbuf); + strbuf_release(&unquoted); strbuf_release(&buf); }
-- 2.7.0.244.g0701a9d