Bug 430827

Summary: shouldn't override user's LS_COLORS
Product: [Fedora] Fedora Reporter: Mikel Ward <mikel>
Component: coreutilsAssignee: Ondrej Vasik <ovasik>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: low Docs Contact:
Priority: low    
Version: 8CC: twaugh
Target Milestone: ---Keywords: Reopened
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: coreutils-6.9-16.fc8 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-03-17 12:23:59 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
colorls.sh which doesn't override user defined LS_COLORS none

Description Mikel Ward 2008-01-29 23:37:31 UTC
/etc/profile.d/colorls.sh overrides my $LS_COLORS environment variable.

If I've already set up my own colors, this script shouldn't clobber them.

The obvious change is to put
[ -n "$LS_COLORS" ] && return
at the top of the script.

Comment 1 Ondrej Vasik 2008-01-31 16:58:12 UTC
Thanks for suggestion...
Actually ... it could cause ls working without colors when no ls aliases would
be defined.
Therefore used
if [ -n "$LS_COLORS" ]; then
  #do not override user LS_COLORS but use them
  alias ll='ls -l --color=tty' 2>/dev/null
  alias l.='ls -d .* --color=tty' 2>/dev/null
  alias ls='ls --color=tty' 2>/dev/null
  exit
fi
With following change built as coreutils-6.10-4.fc9 in rawhide. Will be in next
update of F8.

Comment 2 Mikel Ward 2008-01-31 20:56:42 UTC
That check doesn't do anything useful.  The test needs to be around the
dircolors stuff.

If you want to keep the ls aliases, then do
if [ -z "$LS_COLORS" ]; then
    [ -f ~/.dircolors.$TERM ] && eval `dircolors ~/.dircolors.$TERM
    ...
fi


Comment 3 Ondrej Vasik 2008-01-31 21:31:29 UTC
Why do you think that this check doesn't do anything useful? 
If not zero length of LS_COLORS then expect LS_COLORS correctly set and use
colored aliases for ls ... otherwise make the rest of script as it was before...
It works on my machine without problems - where you see the problem? Your
LS_COLORS will be not overwritten by colorls.sh and you will have colored ls as
you have specified in LS_COLORS... 

Comment 4 Mikel Ward 2008-01-31 22:21:30 UTC
The problem is these lines:
COLORS=/etc/DIR_COLORS
[ -e "/etc/DIR_COLORS.$TERM" ] && COLORS="/etc/DIR_COLORS.$TERM"
[ -e "$HOME/.dircolors" ] && COLORS="$HOME/.dircolors"
[ -e "$HOME/.dircolors.$TERM" ] && COLORS="$HOME/.dircolors.$TERM"
[ -e "$HOME/.dir_colors" ] && COLORS="$HOME/.dir_colors"
[ -e "$HOME/.dir_colors.$TERM" ] && COLORS="$HOME/.dir_colors.$TERM"
[ -e "$COLORS" ] || return

They override my LS_COLORS if I have already set it.

Comment 5 Mikel Ward 2008-01-31 23:03:35 UTC
Actually it's this line:
eval `dircolors --sh "$COLORS"`
but I'd wrap all that in the LS_COLORS test.



Comment 6 Ondrej Vasik 2008-02-01 06:34:03 UTC
Created attachment 293693 [details]
colorls.sh which doesn't override user defined LS_COLORS

I know what overrides LS_COLORS - and current version in RAWHIDE DOES NOT
override LS_COLORS ... I think the best way is to attach the shell script... I
think you don't understand where I added lines from comment #1.

It is more clear now? I know that there is now doubled code, but anyway I have
to rework colorls.sh (which is quiet old - as were DIR_COLORS shipped until
coreutils 6.10) because of 256 color support, so I will try to improve it
somehow.

Comment 7 Mikel Ward 2008-02-01 07:46:34 UTC
You don't understand.

Try running dircolors ~/.dircolor.  You will see it prints commands to set
LS_COLORS.

Running eval `dircolors ~/.dircolors` runs the commands printed by dircolors
~/.dircolors.

It is very clear that dircolors is setting LS_COLORS.  My environment sets
LS_COLORS before /etc/profile.d/colorls.sh is run, so my custom LS_COLORS
setting is lost.

Comment 8 Ondrej Vasik 2008-02-02 09:50:36 UTC
Ok, so maybe I don't understand... so please try to explain WHERE I'm using eval
`dircolors ~/.dircolors` in RAWHIDE version of colorls.sh in the case that you
have your LS_COLORS set. I'm sorry for typo in attachment(this version has -z
instead of -n), but in comment #1 is correct sequence used in .sh script shipped
in RAWHIDE branch(coreutils-6.10-4.fc9) and attachment was here only for
clarification where the code from comment #1 is used.
First lines of the RAWHIDE colorls.sh (with additional comments for COLOR_LS
value set):
# color-ls initialization      


if [ -n "$LS_COLORS" ]; then 
  # if your LS_COLORS variable has nonzero length
  # do not override user defined colors and use them
  
  #set aliases for colored ls 
  alias ll='ls -l --color=tty' 2>/dev/null
  alias l.='ls -d .* --color=tty' 2>/dev/null
  alias ls='ls --color=tty' 2>/dev/null

  #THIS WILL EXIT FROM THE SHELL SCRIPT
  return  
fi
# you will never get there when you had LS_COLORS set from e.g. ~/.bashrc
# (because of non zero length "if" with return)
... rest of the shell script which will get LS_COLORS from e.g. /etc/DIR_COLORS 

Could you please clarify where you see the line which will override your LS_COLORS? 

Comment 9 Mikel Ward 2008-02-02 11:02:35 UTC
Ah, sorry, I missed the exit in comment 1 (and the corresponding return in the
new version).

To me the following layout would make more sense:
if [ -z "$LS_COLORS" ]; then
    COLORS=...

    eval `dircolors --sh "$COLORS"`
fi

if [ -n "$LS_COLORS" ]; then
    alias ll=...

fi



Comment 10 Ondrej Vasik 2008-02-02 13:06:01 UTC
As I said in comment #6 , colorls.sh(.csh) has to be rewritten because of 256
color support and because of broken design of DIR_COLORS.xterm usage ... this
file was created for light background (default in XTERM), but TERM type XTERM is
used by many others terminals(e.g. gnome terminal with default black
background). I will see if your layout from comment #9 will be usable in
reworked colorls.sh, but definitely is better than the old one. Thanks for
suggestion...

Comment 11 Fedora Update System 2008-02-21 02:52:33 UTC
coreutils-6.9-13.fc8 has been pushed to the Fedora 8 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update coreutils'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F8/FEDORA-2008-1844

Comment 12 Fedora Update System 2008-02-28 21:46:18 UTC
coreutils-6.9-13.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 13 Ondrej Vasik 2008-03-11 13:29:22 UTC
Sorry, I have to reopen the bug because of the complaints about the current
behaviour. The old one .sh/.csh script overrides user defined LS_COLORS, but
makes LS_COLORS flexible - you may have basic LS_COLORS from /etc/DIR_COLORS and
once you will start xterm, they will get overriden by /etc/DIR_COLORS.xterm .
This is broken by the new version . So I have to remove those changes. 
What do you think about the possibility of env.variable like USER_LS_COLORS -
which will block overwriting of LS_COLORS? This is more suitable solution. Let
me know about your opinion - but I can't use current approach with [ -z
LS_COLORS ];.

Comment 14 Mikel Ward 2008-03-11 21:03:29 UTC
So your use case is:
1. /etc/DIR_COLORS is sourced when you log in to X (where TERM is empty)
2. you start an xterm and want /etc/DIR_COLORS.xterm to be run

One thought is to only run dircolors (and hence set LS_COLORS) if TERM is set. 
This could be done by wrapping the whole thing in an "if [ -n "$TERM" ]; then"
or similar.

Comment 15 Ondrej Vasik 2008-03-12 08:21:15 UTC
No, other user case is:
1) runlevel 3 (TERM is linux(not empty), uses /etc/DIR_COLORS ) 
2) startx (and then start xterm) - TERM is xterm, 
   should use /etc/DIR_COLORS.xterm
but the behaviour is basically same like you described...

Previous solution with [ -n $LS_COLORS ] is wrong and I don't see any suitable
solution - as you should be able to switch between /etc/DIR_COLORS.xterm,
/etc/DIR_COLORS (and in devel branch additionally with /etc/DIR_COLORS.256colors
if the term is able to support 256 colors). So rely only on the TERM variable is
not usable idea - basically the same as the [ -n $LS_COLORS ] . Solution via
additional env.variable will work in all cases - but if used, should be
documented somehow ( I used that approach in latest builds, to make old
behaviour running again ). At any case - you could always add export
LS_COLORS=<your user settings> to the .bashrc , which is executed after shell
scripts from /etc/profile.d/ (like colorls.sh) - and override settings from
colorls shell script. I like the whole idea of keeping user defined LS_COLORS
variable, but I don't see any easy solution without additional env.variable ("if
[ -z USER_LS_COLORS ]; then <override LS_COLORS script> fi" sounds ok for me -
it makes you able to keep LS_COLORS if you wish) .

Comment 16 Mikel Ward 2008-03-12 08:46:42 UTC
Testing whether $TERM is set would work in those situations:
1) TERM is set, LS_COLORS is unset => run dircolors to set LS_COLORS
2) I think startx starts bash as a login shell, which would erase the previous
environment, and thus /etc/profile.d/colorls.sh would be run for the first time
when the xterm is started, but I could be wrong (I don't have a Linux box in
front of me to test)

The only other case to consider is if you start a terminal from inside another
one, e.g. run gnome-terminal inside an xterm.

What I find most problematic is that it runs from /etc/profile as well as
/etc/bashrc.  I will get more evidence and then elaborate.

Comment 17 Mikel Ward 2008-03-17 12:10:14 UTC
OK.  I was relying on .zshenv not getting clobbered by /etc/zprofile and /etc/zshrc.

I've now got rid of .zshenv and made my zsh setup more like bash, and I can deal
with the current setup.

Thanks for your help.

Comment 18 Ondrej Vasik 2008-03-17 12:23:59 UTC
Ok, closing once more time CURRENTRELEASE as there is now a way to keep user
defined LS_COLORS via env. variable - which should be harmless for the rest
unless is set.