Hi Junio On 21/11/2022 00:34, Junio C Hamano wrote: > Philip Oakley <philipoakley@iee.email> writes: > >> Instead of replacing with "..", replace with the empty string, >> implied by passing NULL, and adjust the padding length calculation. > What's the point of saying "implied by passing NULL" here? Is it an > excuse for passing NULL when passing "" would have sufficed and been > more natural, or something? Passing the empty string was my first approach, however Taylor had commented "why pass the empty string, when NULL will do", hence I checked, and yes, we can pass NULL, so I followed that guidance on the re-roll. > Also, it is unclear to whom you are > passing the NULL. I think that it is sufficient that you said > "replace with the empty string" there. I could drop the commit message comment, and keep the NULL being passed tostrbuf_utf8_replace to indicate the empty string, though that may create the same reviewer question that Taylor had. > >> Extend the existing tests for these pretty formats to include >> `Trunc` and Ltrunc` options matching the `trunc` and `ltrunc` >> tests. > A more important thing to say is that we add Trunc and Ltrunc, than > we test for these new features ;-) That would be to swap the paragraphs about, yes? > > You may also want to explain why there is no matching Mtrunc added. Can do, though it felt obvious that the original mtrunc ellipsis would be necessary for the mid-case. > > I also have another comment on the design. > > Imagine there are series of wide characters, each occupying two > display columns, and you give 6 display columns to truncate such a > string into. "trunc" would give you "[][].." (where [] denotes one > such wide letter that occupies two display columns), and "Trunc" > would give you "[][][]". Now if you give only 5 display columns, > to fill instead of 6, what should happen? My reading of the existing code for ltrunc/mtrunc/trunc was that all these padding conditions were already covered. It was simply a matter of column counting. > > I do not recall how ".."-stuffed truncation works in this case but > it should notice that it cannot stuff 3 wide letters and give you > "[][].". The current code may be already buggy, but at least at the > design level, it is fairly clear what the feature _should_ do. > > As a design question, what should "Trunc" do in such a case now? I > do not think we can still call it "hard truncate" if the feature > gives "[][]" (i.e. fill only 4 display columns, resulting in a > string that is not wide enough) or "[][][]" (i.e. exceed 5 columns > that are given), but of course chomping a letter in the middle is > not acceptable behaviour, so ... The design had already covered those cases. The author already had those thoughts -- Philip
