Ubuntu

diffstat does not honor COLUMNS environment variable

Reported by Michael B. Trausch on 2008-03-31
6
Affects Status Importance Assigned to Milestone
diffstat (Ubuntu)
Undecided
François Marier

Bug Description

Binary package hint: diffstat

The 'diffstat' program does not honor the COLUMNS environment variable, so invocations like:

  $ diff -u some_dir/ some_dir.new/ | COLUMNS=72 diffstat

yield unexpected results. Debdiff and diffstat forthcoming.

Related branches

Michael B. Trausch (mtrausch) wrote :

diffstat:

 debian/patches/00list | 1
 debian/patches/01-check-columns-env-var.dpatch | 27 +++++++++++++++++
 diffstat-1.45/debian/changelog | 10 ++++++
 diffstat-1.45/debian/control | 5 +--
 diffstat-1.45/debian/rules | 9 ++++-
 5 files changed, 48 insertions(+), 4 deletions(-)

Michael B. Trausch (mtrausch) wrote :

confirming prior to subscribing sponsorship team

Changed in diffstat:
status: New → Confirmed
Daniel Holbach (dholbach) wrote :

Steve: can you take a look at it?

Michael: can you also forward the patch upstream?

  • unnamed Edit (189 bytes, application/pgp-signature; name=signature.asc)

On Mon, 2008-03-31 at 06:25 +0000, Daniel Holbach wrote:
> Michael: can you also forward the patch upstream?

Sorry, I forgot to mention that I had done that, too. I have emailed
the bare patch to the author of the diffstat program. I did both Ubuntu
and the upstream author at the same time. I have not heard back from
the upstream author yet, however.

 --- Mike

--
Michael B. Trausch <email address hidden>
home: 404-592-5746, 1 www.trausch.us
cell: 678-522-7934 im: <email address hidden>, jabber
Ubuntu Unofficial Backports Project: http://backports.trausch.us/

Steve Langasek (vorlon) wrote :

Hi Michael,

While it may seem obvious that we would want diffstat to use the $COLUMNS value from the environment, this change is a behavior change for anyone running diffstat in a terminal window that's not 80 columns wide, which may have unexpected side effects for tools that invoke diffstat. So I think it's better to defer this change until after the hardy release.

In the meantime, perhaps you could consider getting this change accepted into the Debian package, so that it will flow into intrepid automatically once the archive unfreezes? Also, you may want to consider whether the decision to honor the $COLUMNS value should be conditional on whether stdout points at a terminal - this is something that packages like, e.g., dpkg do, to good effect.

Michael B. Trausch (mtrausch) wrote :
  • unnamed Edit (189 bytes, application/pgp-signature; name=signature.asc)

On Mon, 2008-03-31 at 20:04 +0000, Steve Langasek wrote:
> While it may seem obvious that we would want diffstat to use the
> $COLUMNS value from the environment, this change is a behavior change
> for anyone running diffstat in a terminal window that's not 80 columns
> wide, which may have unexpected side effects for tools that invoke
> diffstat. So I think it's better to defer this change until after the
> hardy release.
>
> In the meantime, perhaps you could consider getting this change
> accepted into the Debian package, so that it will flow into intrepid
> automatically once the archive unfreezes? Also, you may want to
> consider whether the decision to honor the $COLUMNS value should be
> conditional on whether stdout points at a terminal - this is something
> that packages like, e.g., dpkg do, to good effect.

I have submitted it upstream for consideration, and I can look into
getting it into Debian. Not sure what that would entail precisely, but
I can look at it and see what I can do with it.

Also you brought up a great point there about considering what stdout
is. I will add that to my patch shortly. Thanks!

--
Michael B. Trausch <email address hidden>
home: 404-592-5746, 1 www.trausch.us
cell: 678-522-7934 im: <email address hidden>, jabber
Ubuntu Unofficial Backports Project: http://backports.trausch.us/

Michael B. Trausch (mtrausch) wrote :

Diffstat on attached patch, which now considers stdout's status as a tty before acting on COLUMNS:

 debian/patches/00list | 1
 debian/patches/01-check-columns-env-var.dpatch | 30 +++++++++++++++++++++++++
 diffstat-1.45/debian/changelog | 12 ++++++++++
 diffstat-1.45/debian/control | 5 ++--
 diffstat-1.45/debian/rules | 9 +++++--
 5 files changed, 53 insertions(+), 4 deletions(-)

Thinking about it more, though, I think I like dpkg's behavior, wherein COLUMNS is honored even when stdout is a process instead of a tty. For example if you run:

  COLUMNS=70 dpkg -l ubuntu-desktop | less

The output is properly formatted, whereas if you remove the COLUMNS=, you'll find that it spans nearly 140 columns. Either way, though, it works.

Steve Langasek (vorlon) on 2008-04-02
Changed in diffstat:
assignee: nobody → vorlon
Thomas Dickey (dickey-his) wrote :

diffstat already has a -w option which can be used to achieve the same result.
Making it respond to the environment variable would change existing uses.

man diffstat

       -w number
              specify the maximum width of the histogram. The histogram will
              never be shorter than 10 columns, just in case the filenames get
              too large.

Steve Langasek (vorlon) wrote :

The request was for diffstat to honor the existing $COLUMNS env variable. Commandline options are orthogonal to this.

On Wed, 20 Jan 2010, Steve Langasek wrote:

> The request was for diffstat to honor the existing $COLUMNS env
> variable. Commandline options are orthogonal to this.

If you have some advice on how to not impact existing usage, you should
comment on that in the bug report.

--
Thomas E. Dickey
http://invisible-island.net
ftp://invisible-island.net

François Marier (fmarier) wrote :

I've just applied the second patch in the 1.51-2 package I uploaded to unstable.

Changed in diffstat (Ubuntu):
status: Confirmed → Fix Committed
assignee: Steve Langasek (vorlon) → François Marier (fmarier)
Thomas Dickey (dickey-his) wrote :

For whatever reason, this change will not appear in upstream,
since it adversely affects existing applications.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package diffstat - 1.52-1

---------------
diffstat (1.52-1) unstable; urgency=low

  * New upstream release:
    - new "-C" option to show the histogram using SGR colors
    - new "-s" option to show only the summary line
  * Drop manpage hyphens patch (applied upstream)

diffstat (1.51-2) unstable; urgency=low

  * Adopt this package (closes: #588876)
  * Consider the COLUMNS environment variable when determining the default
    output width (LP: #209537). Thanks to Michael B. Trausch!
  * Add a patch to fix manpage hyphens (lintian notice)

  * Add homepage field to debian/control
  * Bump Standards Version to 3.9.0 and debhelper version to 7
  * Mention git repo in debian/control
  * Add a watch file for the ftp site
  * Switch to 3.0 (quilt) source package format
  * Use a minimal debian/rules makefile

diffstat (1.51-1) unstable; urgency=low

  * New upstream release.
    - Automagic detection and handling of compressed diffs. (closes: 491575)
    - Handle special case of no-newline message from some diffs. (closes: 563875)
  * Orphan this package (set maintainer to Debian QA Group).
 -- Alessio Treglia <email address hidden> Sat, 17 Jul 2010 10:35:59 +1200

Changed in diffstat (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments