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
