| Message ID | 20181208163647.19538-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: > In addition to old %(trailers:only) it is now allowed to write > %(trailers:only=yes) s/$/. Similarly the unfold option can take a boolean./ > 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. Makes sense. > +** '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. > +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, > + const char **end, int *val) > +{ > + const char *p; > + if (!skip_prefix(to_parse, candidate, &p)) > + return 0; > + > + if (match_placeholder_arg(p, "=no", end) || > + match_placeholder_arg(p, "=off", end) || > + match_placeholder_arg(p, "=false", end)) { > + *val = 0; > + return 1; > + } > + > + if (match_placeholder_arg(p, "", end) || > + match_placeholder_arg(p, "=yes", end) || > + match_placeholder_arg(p, "=on", end) || > + match_placeholder_arg(p, "=true", end)) { > + *val = 1; > + return 1; > + } Hmph. Is there a possibility to arrenge the code so that we do not have to maintain these variants of true/false representations here, when we should already have one in config.c? The match_placeholder_arg() function is a bit too limiting as it can only recognize the value that we know about for a thing like this. Instead, perhaps we can cut what follows "=" syntactically, looking for either NUL, ',', or ')', and then call git_parse_maybe_bool() on it. That way, we can handle %(trailers:only=bogo) more sensibly, no? Syntactically we can recognize that the user wanted to give 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if we did so. > + return 0; > +} > +
Junio C Hamano writes: > That way, we can handle %(trailers:only=bogo) more sensibly, > no? Syntactically we can recognize that the user wanted to give > 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if > we did so. I agree that proper error reporting for the pretty formatting strings would be great. But that would depart from the current extremely crude error handling where incorrect formatting placeholders are just left unexpanded. How would such change in error handling be done safely, wrt backwards compatibility changes? To get good diagnostics for incorrect formatting strings I think the way forward is to have the formatting strings parsed once into some kind of AST or machine (as also mentioned by Jeff) that is just executed many times, instead of parsed each time like today. anders
On Tue, Dec 18, 2018 at 10:30:04PM +0100, Anders Waldenborg wrote: > > Junio C Hamano writes: > > That way, we can handle %(trailers:only=bogo) more sensibly, > > no? Syntactically we can recognize that the user wanted to give > > 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if > > we did so. > > I agree that proper error reporting for the pretty formatting strings > would be great. But that would depart from the current extremely crude > error handling where incorrect formatting placeholders are just left > unexpanded. How would such change in error handling be done safely, wrt > backwards compatibility changes? I think we'd want to move in the direction of enforcing valid expressions for %(foo) placeholders. There's some small value in leaving %X alone if we do not understand "X" (not to mention the backwards %compatibility you mentioned), but I think %() is a pretty deliberate indication that a placeholder was meant there. We already do this for ref-filter expansions: $ git for-each-ref --format='%(foo)' fatal: unknown field name: foo We don't for "--pretty" formats, but I do wonder if anybody would be really mad (after all, we have declared ourselves free to add new placeholders, so such formats are not future-proof). -Peff
Jeff King writes: > There's some small value in leaving > %X alone if we do not understand "X" (not to mention the backwards > %compatibility you mentioned), but I think %() is a pretty > deliberate indication that a placeholder was meant there. Good point. > We already do this for ref-filter expansions: > > $ git for-each-ref --format='%(foo)' > fatal: unknown field name: foo > > We don't for "--pretty" formats, but I do wonder if anybody would be > really mad (after all, we have declared ourselves free to add new > placeholders, so such formats are not future-proof). Oh my. I wasn't aware that there was a totally separate string interpolation implementation used for ref filters. That one has separated parsing, making it more amenable to good error handling. I wonder if that could be generalized and reused for pretty formats. However I doubt I will have time to dig deeper into that in near time.
Оля Тележная writes: >> Oh my. I wasn't aware that there was a totally separate string >> interpolation implementation used for ref filters. That one has >> separated parsing, making it more amenable to good error handling. >> I wonder if that could be generalized and reused for pretty formats. >> >> However I doubt I will have time to dig deeper into that in near time. >> > > Sorry, I haven't read your patch in details. If you will be at Git Merge > tomorrow, you could ask me any questions, I can explain how for-each-ref > formatting works amd maybe even give you some ideas how to use its logic in > pretty, I was thinking about it a bit. No, unfortunately I'm not at Git Merge. I think I got how the formatting work. But if you have ideas on how to reuse the logic I'm all ears.
чт, 31 янв. 2019 г. в 21:47, Anders Waldenborg <anders@0x63.nu>: > > > Оля Тележная writes: > >> Oh my. I wasn't aware that there was a totally separate string > >> interpolation implementation used for ref filters. That one has > >> separated parsing, making it more amenable to good error handling. > >> I wonder if that could be generalized and reused for pretty formats. > >> > >> However I doubt I will have time to dig deeper into that in near time. > >> > > > > Sorry, I haven't read your patch in details. If you will be at Git Merge > > tomorrow, you could ask me any questions, I can explain how for-each-ref > > formatting works amd maybe even give you some ideas how to use its logic in > > pretty, I was thinking about it a bit. > > No, unfortunately I'm not at Git Merge. > > I think I got how the formatting work. But if you have ideas on how to > reuse the logic I'm all ears. Maybe my advices will be not suitable: I know how ref-filter works, but I know only a few about pretty system. The main point is that we need to save backward compatibility, and that means that we can't just drop pretty logic and start using ref-filter one there. I guess it's not so hard to make like translation table between pretty commands and ref-filter one. For example, 'short' in pretty means 'commit %(objectname)%0aAuthor: %(author)' in ref-filter. So if I wanted to change pretty logic, I would add support of all ref-filter commands and make translation table for backward compatibility. Hope it was helpful. >
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..26efdba73a 100644 --- a/pretty.c +++ b/pretty.c @@ -1074,6 +1074,31 @@ 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 *p; + if (!skip_prefix(to_parse, candidate, &p)) + return 0; + + if (match_placeholder_arg(p, "=no", end) || + match_placeholder_arg(p, "=off", end) || + match_placeholder_arg(p, "=false", end)) { + *val = 0; + return 1; + } + + if (match_placeholder_arg(p, "", end) || + match_placeholder_arg(p, "=yes", end) || + match_placeholder_arg(p, "=on", end) || + match_placeholder_arg(p, "=true", end)) { + *val = 1; + return 1; + } + + return 0; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1318,11 +1343,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 | 32 +++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 18 ++++++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-)