Re: [PATCH v2 1/2] t4205, t6006: Add failing tests for the case when i18n.logOutputEncoding is set

From: Alexey Shumkin <alex.crezoff@gmail.com>
Date: May 17, 2014, 08:52 UTC
Message-ID: <20140517085203.GA27165@dell-note>
In-Reply-To: <xmqqfvk9a797.fsf@gitster.dls.corp.google.com>

On Fri, May 16, 2014 at 11:49:40AM -0700, Junio C Hamano wrote:

Show 36 quoted lines

> Alexey Shumkin <alex.crezoff@gmail.com> writes:
> 
> > Pretty format string %<(N,[ml]trunc)>%s truncates subject to a given
> > length with an appropriate padding. This works for non-ASCII texts when
> > i18n.logOutputEncoding is UTF-8 only (independently of a printed commit
> > message encoding) but does not work when i18n.logOutputEncoding is NOT
> > UTF-8.
> >
> > There were no breakages as far as were no tests for the case
> > when both a commit message and logOutputEncoding are not UTF-8.
> >
> > Add failing tests for that which will be fixed in the next patch.
> >
> > Signed-off-by: Alexey Shumkin <Alex.Crezoff@gmail.com>
> > Reviewed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> > ---
> >  t/t4205-log-pretty-formats.sh | 169 ++++++++++++++++++++++++++++++++++++++++++
> >  t/t6006-rev-list-format.sh    |  75 ++++++++++++++++++-
> >  2 files changed, 242 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> > index 2a6278b..6791e0d 100755
> > --- a/t/t4205-log-pretty-formats.sh
> > +++ b/t/t4205-log-pretty-formats.sh
> > @@ -153,6 +153,19 @@ EOF
> >  	test_cmp expected actual
> >  '
> >  
> > +test_expect_success 'left alignment formatting. i18n.logOutputEncoding' '
> > +	git -c i18n.logOutputEncoding=iso8859-1 log --pretty="format:%<(40)%s" >actual &&
> > +	# complete the incomplete line at the end
> > +	echo >>actual &&
> 
> Would it change the meaning of the test if you used tformat: instead
> of format: (or --format="%<(40)%s")?  If it doesn't, it would make
> it unnecessary to append an extra LF and explain why you do so.

Well, actually, I just copied previous tests and added i18n.logOutputEncoding.

But as I can see in the code - no, tformat will not change the meaning. so, may be there is a reason to change that (initial) tests from format to tformat first? And then add mine new.

Show 9 quoted lines

> 
> > +	qz_to_tab_space <<EOF | iconv -f utf-8 -t iso8859-1 >expected &&
> 
> It is minor but many existing uses of iconv in our tests spell these
> as UTF-8 and ISO8859-1 in uppercase.  I vaguely recall there was a
> portability concern to favor the ones that are used in existing
> tests, but probably it no longer matters (I see you added the
> lowercase one with de6029a2 mid last year), so I am fine if these
> stay lowercase.

I've grep'ed for lowercase iso8859-1 in test, and found almost all of them is code added by me. Grep for uppercase gives more results. I can refactor that first for uniformity.

Show 6 quoted lines

> 
> > +	git -c i18n.logOutputEncoding=iso8859-1 log --pretty="format:%<(1)%s" >actual &&
> > +	# complete the incomplete line at the end
> > +	echo >>actual &&
> 
> Likewise for all the other "--pretty=format:" followed by an echo.

the same copy-paste-modify.

> 
> Thanks.

-- 
Alexey Shumkin