| Message ID | 20221030185614.3842-2-philipoakley@iee.email (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | extend the truncating pretty formats | expand |
On Sun, Oct 30, 2022 at 06:56:14PM +0000, Philip Oakley wrote: > Instead of replacing with "..", replace with the empty string "", > having adjusted the padding length calculation. > > Needswork: > There are no tests for these pretty formats, before or after > this change. Hmmph. I see some when searching for l?trunc in t4205-log-pretty-formats.sh. Is that coverage not sufficient for the existing feature? If so, it would be nice to see it bulked up to add coverage where we are missing some. Either way, we should make sure the new code is covered before continuing. > @@ -1743,6 +1749,16 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ > padding - 2, len - (padding - 2), > ".."); > break; > + case trunc_left_hard: > + strbuf_utf8_replace(&local_sb, > + 0, len - (padding), > + ""); > + break; > + case trunc_right_hard: > + strbuf_utf8_replace(&local_sb, > + padding, len - (padding), > + ""); > + break; If I remember correctly, strbuf_utf8_replace() supports taking NULL as its last argument, though upon searching I couldn't find any callers that behave that way. Can we use that instead of supplying the empty string? If not, should we drop support for the NULL-as-last-argument? Thanks, Taylor
On 30/10/2022 19:23, Taylor Blau wrote: > On Sun, Oct 30, 2022 at 06:56:14PM +0000, Philip Oakley wrote: >> Instead of replacing with "..", replace with the empty string "", >> having adjusted the padding length calculation. >> >> Needswork: >> There are no tests for these pretty formats, before or after >> this change. > Hmmph. I see some when searching for l?trunc in > t4205-log-pretty-formats.sh. Is that coverage not sufficient for the > existing feature? > > If so, it would be nice to see it bulked up to add coverage where we > are missing some. Either way, we should make sure the new code is > covered before continuing. No idea how I missed them. Maybe I'd filtered, by accident, the search by a too narrow list of file types. Thanks for that pointer. >> @@ -1743,6 +1749,16 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ >> padding - 2, len - (padding - 2), >> ".."); >> break; >> + case trunc_left_hard: >> + strbuf_utf8_replace(&local_sb, >> + 0, len - (padding), >> + ""); >> + break; >> + case trunc_right_hard: >> + strbuf_utf8_replace(&local_sb, >> + padding, len - (padding), >> + ""); >> + break; > If I remember correctly, strbuf_utf8_replace() supports taking NULL as > its last argument, though upon searching I couldn't find any callers > that behave that way. Can we use that instead of supplying the empty > string? If not, should we drop support for the NULL-as-last-argument? I wasalso concerned about zero length strings but my brief look at the code indicated it could cope with a zero length string. The last param is `const char *subst`. I've just relooked at the code and it does have a if (subst) subst_len = strlen(subst); so it does look safe to pass NULL, though it would probably cause a pause for thought for readers, and isn't likely to be that much faster in these format scenarios. > > Thanks, > Taylor -- Philip
On Sun, Oct 30, 2022 at 10:01:47PM +0000, Philip Oakley wrote: > > If I remember correctly, strbuf_utf8_replace() supports taking NULL as > > its last argument, though upon searching I couldn't find any callers > > that behave that way. Can we use that instead of supplying the empty > > string? If not, should we drop support for the NULL-as-last-argument? > > I wasalso concerned about zero length strings but my brief look at the > code indicated it could cope with a zero length string. > The last param is `const char *subst`. > > I've just relooked at the code and it does have a > > if (subst) > subst_len = strlen(subst); > > so it does look safe to pass NULL, though it would probably cause a > pause for thought for readers, and isn't likely to be that much faster > in these format scenarios. I'm not worried at all about speed, I was just wondering if there was a more designated helper for "truncate these many UTF-8 characters from the end of a string". It appears that passing NULL to that argument behaves the same as passing "", so I was wondering if it would be clearer to use that. But I'm fine either way. If there are no callers that pass NULL, then we should consider something like the following: --- 8< --- diff --git a/utf8.c b/utf8.c index de4ce5c0e6..e8813f64fe 100644 --- a/utf8.c +++ b/utf8.c @@ -361,10 +361,9 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width, char *src = sb_src->buf; char *end = src + sb_src->len; char *dst; - int w = 0, subst_len = 0; + int w = 0, subst_len; - if (subst) - subst_len = strlen(subst); + subst_len = strlen(subst); strbuf_grow(&sb_dst, sb_src->len + subst_len); dst = sb_dst.buf; --- >8 --- Below the context there is another "if (subst)", but that one needs to stay since we only perform the replacement once. Thanks, Taylor
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 0b4c1c8d98..bd1657c032 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -146,14 +146,15 @@ The placeholders are: '%m':: left (`<`), right (`>`) or boundary (`-`) mark '%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of linkgit:git-shortlog[1]. -'%<(<N>[,trunc|ltrunc|mtrunc])':: make the next placeholder take at +'%<(<N>[,trunc|ltrunc|mtrunc|Trunc|Ltrunc])':: make the next placeholder take at least N columns, padding spaces on the right if necessary. Optionally - truncate at the beginning (ltrunc), + truncate (with ellipsis '..') at the beginning (ltrunc), the middle (mtrunc) or the end (trunc) if the output is longer than - N columns. Note that truncating + N columns. Note that truncating with ellipsis only works correctly with N >= 2. + Use (Trunc|Ltrunc) for hard truncation without ellipsis. '%<|(<N>)':: make the next placeholder take at least until Nth columns, padding spaces on the right if necessary '%>(<N>)', '%>|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' respectively, diff --git a/pretty.c b/pretty.c index 6cb363ae1c..f67844d638 100644 --- a/pretty.c +++ b/pretty.c @@ -857,7 +857,9 @@ enum trunc_type { trunc_none, trunc_left, trunc_middle, - trunc_right + trunc_right, + trunc_left_hard, + trunc_right_hard }; struct format_commit_context { @@ -1145,6 +1147,10 @@ static size_t parse_padding_placeholder(const char *placeholder, c->truncate = trunc_left; else if (starts_with(start, "mtrunc)")) c->truncate = trunc_middle; + else if (starts_with(start, "Ltrunc)")) + c->truncate = trunc_left_hard; + else if (starts_with(start, "Trunc)")) + c->truncate = trunc_right_hard; else return 0; } else @@ -1743,6 +1749,16 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ padding - 2, len - (padding - 2), ".."); break; + case trunc_left_hard: + strbuf_utf8_replace(&local_sb, + 0, len - (padding), + ""); + break; + case trunc_right_hard: + strbuf_utf8_replace(&local_sb, + padding, len - (padding), + ""); + break; case trunc_none: break; }
Instead of replacing with "..", replace with the empty string "", having adjusted the padding length calculation. Needswork: There are no tests for these pretty formats, before or after this change. Signed-off-by: Philip Oakley <philipoakley@iee.email> --- Documentation/pretty-formats.txt | 7 ++++--- pretty.c | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 4 deletions(-)