| Message ID | 20190128213337.24752-3-anders@0x63.nu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | %(trailers) improvements in pretty format | expand |
Anders Waldenborg <anders@0x63.nu> writes: > Subject: Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value Style: s/pretty: Allow/pretty: allow/ (haven't I said this often enough?) > +** 'only[=val]': select whether non-trailer lines from the trailer > + block should be included. The `only` keyword may optionally be > + followed by an equal sign and one of `true`, `on`, `yes` to omit or > + `false`, `off`, `no` to show the non-trailer lines. If option is > + given without value it is enabled. If given multiple times the last > + value is used. > +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` > + option was given. In same way as to for `only` it can be followed > + by an equal sign and explicit value. E.g., > + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. Sounds sensible. > diff --git a/pretty.c b/pretty.c > index b83a3ecd23..b8d71a57c9 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb, > return 0; > } > > -static int match_placeholder_arg(const char *to_parse, const char *candidate, > - const char **end) > +static int match_placeholder_arg_value(const char *to_parse, const char *candidate, > + const char **end, const char **valuestart, size_t *valuelen) An overlong line here. > ... > +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, > + const char **end, int *val) > +{ > + char buf[8]; > + const char *strval; > + size_t len; > + int v; > + > + if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len)) > + return 0; > + > + if (!strval) { > + *val = 1; > + return 1; > + } > + > + strlcpy(buf, strval, sizeof(buf)); > + if (len < sizeof(buf)) > + buf[len] = 0; Doesn't strlcpy() terminate buf[len] if len is short enough? Even if the strval is longer than buf[], strlcpy() would truncate and make sure buf[] is NUL terminated, no? > + v = git_parse_maybe_bool(buf); Why? This function would simply be buggy and incapable of parsing a representation of a boolean value that is longer than 8 bytes (if such a representation exists), so chomping an overlong string at the end and feeding it to git_parse_maybe_bool() is a nonsense, isn't it? In this particular case, strlcpy() is inviting a buggy programming. If there were a 7-letter representation of falsehood, strval may be that 7-letter thing, in which case you would want to feed it to git_parse_maybe_bool() to receive "false" from it, or strval may have that 7-letter thing followed by a 'x' (so as a token, that is not a correctly spelled falsehood), but strlcpy() would chomp and show the same 7-letter falsehood to git_parse_maybe_bool(). That robs you from an opportunity to diagnose such a bogus input as an error. Instead of using "char buf[8]", just using a strbuf and avoidng strlcpy() would make the code much better, I would think.
Junio C Hamano writes: >> ... >> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, >> + const char **end, int *val) >> +{ >> + char buf[8]; >> + const char *strval; >> + size_t len; >> + int v; >> + >> + if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len)) >> + return 0; >> + >> + if (!strval) { >> + *val = 1; >> + return 1; >> + } >> + >> + strlcpy(buf, strval, sizeof(buf)); >> + if (len < sizeof(buf)) >> + buf[len] = 0; > > Doesn't strlcpy() terminate buf[len] if len is short enough? > Even if the strval is longer than buf[], strlcpy() would truncate > and make sure buf[] is NUL terminated, no? Yes, but no. strval is not NUL-terminated at len. E.g strval would point to "false,something=true". `buf[len] = 0` makes sure it becomes "false". > Instead of using "char buf[8]", just using a strbuf and avoidng > strlcpy() would make the code much better, I would think. Yes, taking the heap allocation hit would most likely make the intent clearer.
In addition to old %(trailers:only) it is now allowed to write
%(trailers:only=yes)
By itself this only gives (the not quite so useful) possibility to have
users change their mind in the middle of a formatting
string (%(trailers:only=true,only=false)). However, it gives users the
opportunity to override defaults from future options.
Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
Documentation/pretty-formats.txt | 14 ++++++---
pretty.c | 52 +++++++++++++++++++++++++++-----
t/t4205-log-pretty-formats.sh | 18 +++++++++++
3 files changed, 73 insertions(+), 11 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 86d804fe97..d33b072eb2 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -225,10 +225,16 @@ endif::git-rev-list[]
linkgit:git-interpret-trailers[1]. The
`trailers` string may be followed by a colon
and zero or more comma-separated options:
-** 'only': omit non-trailer lines from the trailer block.
-** 'unfold': make it behave as if interpret-trailer's `--unfold`
- option was given. E.g., `%(trailers:only,unfold)` unfolds and
- shows all trailer lines.
+** 'only[=val]': select whether non-trailer lines from the trailer
+ block should be included. The `only` keyword may optionally be
+ followed by an equal sign and one of `true`, `on`, `yes` to omit or
+ `false`, `off`, `no` to show the non-trailer lines. If option is
+ given without value it is enabled. If given multiple times the last
+ value is used.
+** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
+ option was given. In same way as to for `only` it can be followed
+ by an equal sign and explicit value. E.g.,
+ `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
NOTE: Some placeholders may depend on other options given to the
revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index b83a3ecd23..4dfbd38cf6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1056,13 +1056,26 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
return 0;
}
-static int match_placeholder_arg(const char *to_parse, const char *candidate,
- const char **end)
+static int match_placeholder_arg_value(const char *to_parse, const char *candidate,
+ const char **end, const char **valuestart,
+ size_t *valuelen)
{
const char *p;
if (!(skip_prefix(to_parse, candidate, &p)))
return 0;
+ if (valuestart) {
+ if (*p == '=') {
+ *valuestart = p + 1;
+ *valuelen = strcspn(*valuestart, ",)");
+ p = *valuestart + *valuelen;
+ } else {
+ if (*p != ',' && *p != ')')
+ return 0;
+ *valuestart = NULL;
+ *valuelen = 0;
+ }
+ }
if (*p == ',') {
*end = p + 1;
return 1;
@@ -1074,6 +1087,34 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
return 0;
}
+static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,
+ const char **end, int *val)
+{
+ const char *argval;
+ char *strval;
+ size_t arglen;
+ int v;
+
+ if (!match_placeholder_arg_value(to_parse, candidate, end, &argval, &arglen))
+ return 0;
+
+ if (!argval) {
+ *val = 1;
+ return 1;
+ }
+
+ strval = xstrndup(argval, arglen);
+ v = git_parse_maybe_bool(strval);
+ free(strval);
+
+ if (v == -1)
+ return 0;
+
+ *val = v;
+
+ return 1;
+}
+
static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
if (*arg == ':') {
arg++;
for (;;) {
- if (match_placeholder_arg(arg, "only", &arg))
- opts.only_trailers = 1;
- else if (match_placeholder_arg(arg, "unfold", &arg))
- opts.unfold = 1;
- else
+ if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
+ !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold))
break;
}
}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66ff..63730a4ec0 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' '
test_cmp expect actual
'
+test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' '
+ git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+ grep -v patch.description <trailers >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no) shows all trailers' '
+ git log --no-walk --pretty=format:"%(trailers:only=no)" >actual &&
+ cat trailers >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' '
+ git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual &&
+ grep -v patch.description <trailers >expect &&
+ test_cmp expect actual
+'
+
test_expect_success '%(trailers:unfold) unfolds trailers' '
git log --no-walk --pretty="%(trailers:unfold)" >actual &&
{
On Tue, Jan 29, 2019 at 07:45:06AM +0100, Anders Waldenborg wrote: > > Instead of using "char buf[8]", just using a strbuf and avoidng > > strlcpy() would make the code much better, I would think. > > Yes, taking the heap allocation hit would most likely make the intent > clearer. If you can reuse the same struct and strbuf_reset() it each time, then that amortizes the cost of the heap (to basically once per program run, instead of once per commit). -Peff
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 86d804fe97..d33b072eb2 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -225,10 +225,16 @@ endif::git-rev-list[] linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by a colon and zero or more comma-separated options: -** 'only': omit non-trailer lines from the trailer block. -** 'unfold': make it behave as if interpret-trailer's `--unfold` - option was given. E.g., `%(trailers:only,unfold)` unfolds and - shows all trailer lines. +** 'only[=val]': select whether non-trailer lines from the trailer + block should be included. The `only` keyword may optionally be + followed by an equal sign and one of `true`, `on`, `yes` to omit or + `false`, `off`, `no` to show the non-trailer lines. If option is + given without value it is enabled. If given multiple times the last + value is used. +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` + option was given. In same way as to for `only` it can be followed + by an equal sign and explicit value. E.g., + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index b83a3ecd23..b8d71a57c9 100644 --- a/pretty.c +++ b/pretty.c @@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb, return 0; } -static int match_placeholder_arg(const char *to_parse, const char *candidate, - const char **end) +static int match_placeholder_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, size_t *valuelen) { const char *p; if (!(skip_prefix(to_parse, candidate, &p))) return 0; + if (valuestart) { + if (*p == '=') { + *valuestart = p + 1; + *valuelen = strcspn(*valuestart, ",)"); + p = *valuestart + *valuelen; + } else { + if (*p != ',' && *p != ')') + return 0; + *valuestart = NULL; + *valuelen = 0; + } + } if (*p == ',') { *end = p + 1; return 1; @@ -1074,6 +1086,35 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, return 0; } +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + char buf[8]; + const char *strval; + size_t len; + int v; + + if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len)) + return 0; + + if (!strval) { + *val = 1; + return 1; + } + + strlcpy(buf, strval, sizeof(buf)); + if (len < sizeof(buf)) + buf[len] = 0; + + v = git_parse_maybe_bool(buf); + if (v == -1) + return 0; + + *val = v; + + return 1; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (*arg == ':') { arg++; for (;;) { - if (match_placeholder_arg(arg, "only", &arg)) - opts.only_trailers = 1; - else if (match_placeholder_arg(arg, "unfold", &arg)) - opts.unfold = 1; - else + if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) break; } } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 978a8a66ff..63730a4ec0 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' ' test_cmp expect actual ' +test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no) shows all trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=no)" >actual && + cat trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + test_expect_success '%(trailers:unfold) unfolds trailers' ' git log --no-walk --pretty="%(trailers:unfold)" >actual && {
In addition to old %(trailers:only) it is now allowed to write %(trailers:only=yes) By itself this only gives (the not quite so useful) possibility to have users change their mind in the middle of a formatting string (%(trailers:only=true,only=false)). However, it gives users the opportunity to override defaults from future options. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 14 ++++++--- pretty.c | 52 +++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 18 +++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-)