Re: [PATCH v4] pretty-formats: add hard truncation, without ellipsis, options

From: Philip Oakley <philipoakley@iee.email>
Date: Nov 21, 2022, 18:10 UTC
Message-ID: <d80d1b97-b0c0-148b-afb7-f5210366e463@iee.email>
In-Reply-To: <xmqqfsedywli.fsf@gitster.g>

Hi Junio

On 21/11/2022 00:34, Junio C Hamano wrote:

Show 7 quoted lines

> 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.

Show 6 quoted lines

>
>> 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.

Show 9 quoted lines

>
> 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.

Show 12 quoted lines

>
> 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