From: Junio C Hamano <gitster@pobox.com>
To: "Zbigniew Jędrzejewski-Szmek" <zbyszek@in.waw.pl>
Cc: git@vger.kernel.org, Michael J Gruber <git@drmicha.warpmail.net>,
pclouds@gmail.com, j.sixt@viscovery.net
Subject: Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
Date: Wed, 22 Feb 2012 11:41:12 -0800 [thread overview]
Message-ID: <7vr4xmac6f.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: 4F44D084.7030308@in.waw.pl
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> by "scales monotonically with the change count" I meant with two
> different commits.
Because of the scaling, the same 300-line change to the same file will be
shown with different number of +- depending on the amount of change to
other files in the same commit. It is not something we should be worried
about to begin with, unless you are going to read all commits in advance
and find necessary length of the name part, and also maxchange across
these commits to align things across commits, which of course would not
mesh well with our desire to stream output whenever possible.
> [7.3/8] limit graph part to 40 columns
Read the problem you are trying to solve again (from your 3/8):
>> If commits changing a lot of lines are displayed in a wide terminal
>> window (200 or more columns), and the +- graph would use the full
>> width, the output would look bad. Messages wrapped to about 80 columns
>> would be interspersed with very long +- lines.
If your name part needs only 10-20 columns and you are used to seeing
graph bars that are 60-70 columns long on your standard 80-column
terminal, you may see it ugly if the same short names are followed by
graph bars that are 180-190 columns (i.e. "very long +-"), when the
messages still fit 80 columns on the standard terminal, which is a width
that can comfortably be scanned with horizontal movement of human eyes. I
would understand that concern, although I do not personally think it is a
big deal.
Imagine if your name part were 125 columns wide (200 * 5/8), and also
imagine you drew the same graph bars as before, i.e. using 60-70 columns.
That line will fill almost the full terminal width, and will extend beyond
the right edge of the message block, but you are not showing "very long +-
lines" anymore. The length of the line comes mostly from overly long
names, carrying more information than before (because your patch lets us
show larger parts of such a long name on wider terminal), and without
losing much information from the graph part.
Now imagine if your patch only needed 10-20 columns for the names, and you
are showing the same change on an 80-column terminal with your 40-column
cap. You will introduce a new problem with "very short +-" lines instead,
as the "diff --stat" block will be much shorter than the surrounding text.
You fixed a bug in the original partitioning with 1/8, which was caused by
a code that capped the length of the graph part too early before taking
the width necessary to show the name into account; the bug resulted in a
graph that did not fill the allotted space fully to the right, even though
the user wanted to give full width to the graph.
The ugliness avoidance against "very long +-" becomes an issue only on
wide terminals; doing the same 40-column cap on non-wide terminals is to
re-introduce the same "cap the graph width too early, ending up with a
graph that is narrower than desired" bug you fixed. Why do you keep
insisting on that broken approach?
It might be just the matter of raising the artificial cap to much higher
than 40-column (say, 80-column). A possible alternative may be to declare
that the perceived ugliness is the user's problem of having overly wide
terminal in the first place and do without any such cap.
Either is fine, but regressing output on 80-column terminal when showing a
patch with short filenames and large changes is unacceptable, not because
I personally use 80-col terminal myself (I don't---mine is a bit wider but
not 200), but because it changes behaviour from the old code without any
good justification to do so.
next prev parent reply other threads:[~2012-02-22 19:41 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-20 21:57 [PATCH 0/8 v6] diff --stat: use the full terminal width Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 1/8 v6] make lineno_width() from blame reusable for others Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 2/8 v6] diff --stat: tests for long filenames and big change counts Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 3/8 v6] diff --stat: use the full terminal width Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 4/8 v6] show " Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 5/8 v6] log " Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 6/8 v6] merge " Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 7/8 v6] diff --stat: limit graph part to 40 columns Zbigniew Jędrzejewski-Szmek
2012-02-20 21:57 ` [PATCH 8/8 v6] diff --stat: use less columns for change counts Zbigniew Jędrzejewski-Szmek
2012-02-20 23:41 ` [PATCH 0/8 v6] diff --stat: use the full terminal width Junio C Hamano
2012-02-21 0:26 ` Zbigniew Jędrzejewski-Szmek
2012-02-21 7:05 ` Junio C Hamano
2012-02-21 10:05 ` Zbigniew Jędrzejewski-Szmek
2012-02-21 20:10 ` Junio C Hamano
2012-02-22 11:24 ` Zbigniew Jędrzejewski-Szmek
2012-02-22 11:51 ` [PATCH 7.1/8] diff --stat: use a maximum of 5/8 for the filename part Zbigniew Jędrzejewski-Szmek
2012-02-22 11:51 ` [PATCH 7.2/8] diff --stat: add a test for output with COLUMNS=40 Zbigniew Jędrzejewski-Szmek
2012-02-22 11:51 ` [PATCH 7.3/8] diff --stat: limit graph part to 40 columns Zbigniew Jędrzejewski-Szmek
2012-02-22 19:41 ` Junio C Hamano [this message]
2012-02-24 20:31 ` [PATCH v7 0/11] diff --stat: use the full terminal width Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 01/11] make lineno_width() from blame reusable for others Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 02/11] diff --stat: tests for long filenames and big change counts Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 03/11] diff --stat: use the full terminal width Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 04/11] show " Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 05/11] log " Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 06/11] merge " Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 07/11] diff --stat: use a maximum of 5/8 for the filename part Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 08/11] diff --stat: add a test for output with COLUMNS=40 Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 09/11] diff --stat: enable limiting of the graph part Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 10/11] diff --stat: add config option to limit graph width Zbigniew Jędrzejewski-Szmek
2012-02-24 20:32 ` [PATCH v7 11/11] diff --stat: use less columns for change counts Zbigniew Jędrzejewski-Szmek
2012-02-21 15:16 ` [PATCH 0/8 v6] diff --stat: use the full terminal width Nguyen Thai Ngoc Duy
2012-02-21 16:11 ` Zbigniew Jędrzejewski-Szmek
2012-02-23 7:29 ` Miles Bader
2012-02-23 5:08 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vr4xmac6f.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=git@drmicha.warpmail.net \
--cc=git@vger.kernel.org \
--cc=j.sixt@viscovery.net \
--cc=pclouds@gmail.com \
--cc=zbyszek@in.waw.pl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).