Re: [GSoC PATCH 2/2] t4074: add test for diffstat width when prefix contains ANSI chars

From: Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>
Date: Feb 25, 2026, 02:18 UTC
Message-ID: <aZ5b5spGgHdMPmH-@lorenzo-VM>
In-Reply-To: <xmqqikbmk86b.fsf@gitster.g>

On Mon, Feb 23, 2026 at 09:43:56PM -0800, Junio C Hamano wrote:

Show 6 quoted lines

> LorenzoPegorari <lorenzo.pegorari2002@gmail.com> writes:
> 
> > Add test checking the calculation of the diffstat display width when
> > the line_prefix contains ANSI characters.
>
> Hmph, who stuffs the "line_prefix" and with what?

Yeah, I should make the commit description clearer. Will improve that.

Show 10 quoted lines

> > Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
> > ---
> >  t/meson.build                               |  1 +
> >  t/t4074-diff-stat-width-with-line-prefix.sh | 42 +++++++++++++++++++++
> >  2 files changed, 43 insertions(+)
> >  create mode 100755 t/t4074-diff-stat-width-with-line-prefix.sh
>
> Do we need a brand new test script only to host just two tests?  Is
> it too cumbersome to modify existing test scripts that already test
> "git log --graph"?

Mmh I guess the test could be added to "t4052-stat-output.sh", which "tests --stat output of various commands". Do you agree that this the correct place? (btw, t4052 seems to me like a quite messy test...)

Show 16 quoted lines

> > +setup () {
> > +   rm -rf * ".git" &&
> > +   git init &&
> > +   git config color.diff always
> > +}
>
> Why?
>
> > +test_expect_success 'check width with max name-width' '
> > +   setup &&
>
> Why?  Shouldn't something like
>
>       test_config color.diff always &&
>
> be sufficient?

This is sufficient, and the setup() func makes no sense now that I know this. Thanks for pointing that out!

Show 9 quoted lines

> > +   touch "${FILE_MAX}" &&
>
> Use of "touch" when you do not care about the timestamp of the
> resulting file is misleading.  If you only care about its existence,
> do something like
>
>       >"${FILE_MAX}" &&
>
> instead.

Ack.

Show 8 quoted lines

> > +   git add . &&
> > +   git commit -m "init" &&
> > +   echo "text" >"${FILE_MAX}" &&
> > +   git add . &&
> > +   git commit -m "text" &&
>
> OK, so we have two commits, one adds an empty file, followed by
> another that adds one line of "text" to the file.

Yes, these are just placeholder commits. We need at least 2 commits to create a graph with "log --graph". I will make them "--allow-empty" and "--allow-empty-message" emphasize that they are placeholders.

Show 6 quoted lines

> > +   git log --graph --stat >out &&
> > +   grep "${FILE} | 1" out
> > +'
>
> Use "test_grep" instead of "grep" to make it easier to debug when
> things break, perhaps?

Ack.

Show 13 quoted lines

> It is unclear what we are exactly looking for.  What's the failure
> mode and how the miscounting of the width of the line-prefix
> contribute to that failure?
>
> We have two commits, "log" without "--reverse" would give the
> creation of an empty file i.e., "file | 0" first and then one line
> addition to the file i.e., "file | 1 +" next.  We only care about
> "${FILE} | 1" existing in the output and we do not bother checking
> what comes at the beginning of the line before "${FILE}" or what
> comes on the line fter "| 1", because we can detect the breakage we
> expect to see only by looking at the middle of the line?  How would
> that work?  The test deserves a bit more comment to help readers who
> wonder about these things.

I will add comments to explain the test better. The goal is to have a file with name that is just the exact length so that the line in the git-log output which includes the diffstat will take all the available space in the terminal (if the diffstat width is calculated correctly). Then we test the diffstat width calculation with the terminal being the correct size for the file name, and then 1 column short. If the file name is fully displayed first, and is then shortened by only 1 character, it means that the diffstat width was correctly calculated.

>       Side note.  I think I know the answer to these questions,
>       but this project is not about me ;-) but other future
>       contributors also would need help when they encounter this
>       test and want to learn what it is testing.

Thank you so much for helping me out without just giving me the solution. It must have taken longer :-)

Show 30 quoted lines

> > +test_expect_success 'check width with longer name-width' '
> > +   setup &&
> > +   touch "${FILE_MAX}+" &&
> > +   git add . &&
> > +   git commit -m "init" &&
> > +   echo "text" >"${FILE_MAX}+" &&
> > +   git add . &&
> > +   git commit -m "text" &&
> > +   git log --graph --stat >out &&
> > +   grep "${FILE_LONGER} | 1" out
> > +'
> > +
> > +test_done
>
> Instead of doing the repository construction twice, you can see the
> same effect by running "git log --graph --stat" on the same history
> with different values exported on COLUMNS environment variable,
> which I think would be easier to see and simpler to debug.
>
> Something along the lines of ...
>
>     COLUMNS=80 git log --graph --stat >out &&
>     test_decode_color out >out.decoded &&
>     test_grep "^<RED>|<RESET> ${FILE_MAX} | 1 <GREEN>+<RESET>$" out.decoded &&
>
>     COLUMNS=79 git log --graph --stat >out &&
>     test_decode_color out >out.decoded &&
>     test_grep "^<RED>|<RESET> ${FILE_TRUNCATED} | 1 <GREEN>+<RESET>$" out.decoded
>
> ... perhaps?

Didn't know about this. Will use it for sure.

Again, thanks a lot for the through review Junio. Just let me know if t4052 (described above) is the correct place for the test!

(Sorry for the double email Junio... why does mutt not automatically add the mailing list to CC while replying... :'[ )