Re: ls --color=always and unterminated last line of output
On 2009-12-31 13:17, Jim Meyering wrote:
> > C de-Avillez wrote:
> >
>> >> Original Ubuntu bug:
>> >> https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/494663
>> >>
>> >> Reporter complains last line of a 'ls --color=always' is unterminated,
>> >> causing grief on a pipe to 'ls -FXR'.
>> >>
> >
> > Here's a potential solution.
> > The ls.c change not pretty, but I think it is correct.
> > I'm not sure this belongs in coreutils-8.3, but I'm considering it.
> >
> >>From bbeb3d4f3db5366fbfed6675e964d47dc9efa9d3 Mon Sep 17 00:00:00 2001
> > From: Jim Meyering
> > Date: Thu, 31 Dec 2009 20:09:06 +0100
> > Subject: [PATCH] ls --color: don't emit a final no-op escape sequence
> >
Current GIT requires more changes in misc/ls-misc. Specifically,
sl-dangle, sl-dangle-3, sl-dangle-4 and sl-dangle-5 .
I have adjusted the patch, and am submitting it for consideration.
Regards,
..C..
From c9ce23aa1830fbe91937d24d849ccae6d5a3c243 Mon Sep 17 00:00:00 2001
From: C de-Avillez
Date: Sat, 30 Jan 2010 16:52:46 -0600
Subject: [PATCH] ls --color: don't emit a final no-op escape sequence
* src/ls.c (main): With --color, avoid emitting the final color-
resetting escape sequence when it would be a no-op.
* tests/ls/color-clear-to-eol: Adjust expected output accordingly.
* tests/ls/color-dtype-dir: Likewise.
* tests/ls/multihardlink: Likewise.
* tests/ls/stat-free-symlinks: Likewise.
* tests/misc/ls-misc: Likewise.
Reported by Jim Avera in
http://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/494663
---
src/ls.c| 10 +-
tests/ls/color-clear-to-eol |2 +-
tests/ls/color-dtype-dir|4
tests/ls/multihardlink | 11 +--
tests/ls/stat-free-symlinks |1 -
tests/misc/ls-misc | 17 -
6 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/src/ls.c b/src/ls.c
index 9ef7eba..680679a 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -1442,7 +1442,15 @@ main (int argc, char **argv)
int j;
if (used_color)
-restore_default_color ();
+{
+ /* Skip the restore when it would be a no-op, i.e.,
+ when left is "\033[" and right is "m". */
+ if (!(color_indicator[C_LEFT].len == 2
+&& memcmp (color_indicator[C_LEFT].string, "\033[", 2) == 0
+&& color_indicator[C_RIGHT].len == 1
+&& color_indicator[C_RIGHT].string[0] == 'm'))
+restore_default_color ();
+}
fflush (stdout);
/* Restore the default signal handling. */
diff --git a/tests/ls/color-clear-to-eol b/tests/ls/color-clear-to-eol
index 96944cb..1be68ca 100755
--- a/tests/ls/color-clear-to-eol
+++ b/tests/ls/color-clear-to-eol
@@ -29,7 +29,7 @@ touch $long_name || framework_failure
e='\33'
color_code='0;31;42'
c_pre="$e[0m$e[${color_code}m"
-c_post="$e[0m$e[K\n$e[m"
+c_post="$e[0m$e[K\n"
printf "$c_pre$long_name$c_post\n" > exp || framework_failure
env TERM=xterm COLUMNS=80 LS_COLORS="*.foo=$color_code" TIME_STYLE=+T \
diff --git a/tests/ls/color-dtype-dir b/tests/ls/color-dtype-dir
index 6532d84..29b872c 100755
--- a/tests/ls/color-dtype-dir
+++ b/tests/ls/color-dtype-dir
@@ -36,7 +36,6 @@ chmod o+t sticky || framework_failure
ls --color=always > out || fail=1
cat -A out > o1 || fail=1
-echo >> o1 || fail=1
mv o1 out || fail=1
cat <<\EOF > exp || fail=1
@@ -44,7 +43,6 @@ cat <<\EOF > exp || fail=1
^[[34;42mother-writable^[[0m$
out$
^[[37;44msticky^[[0m$
-^[[m
EOF
compare out exp || fail=1
@@ -56,7 +54,6 @@ rm exp
LS_COLORS="ow=:" ls --color=always > out || fail=1
cat -A out > o1 || fail=1
-echo >> o1 || fail=1
mv o1 out || fail=1
cat <<\EOF > exp || fail=1
@@ -64,7 +61,6 @@ cat <<\EOF > exp || fail=1
^[[01;34mother-writable^[[0m$
out$
^[[37;44msticky^[[0m$
-^[[m
EOF
compare out exp || fail=1
diff --git a/tests/ls/multihardlink b/tests/ls/multihardlink
index 60bb399..20a94c8 100755
--- a/tests/ls/multihardlink
+++ b/tests/ls/multihardlink
@@ -30,7 +30,6 @@ code_mh='44;37'
code_ex='01;32'
code_png='01;35'
c0=$(printf '\033[0m')
-c_end=$(printf '\033[m')
c_mh=$(printf '\033[%sm' $code_mh)
c_ex=$(printf '\033[%sm' $code_ex)
c_png=$(printf '\033[%sm' $code_png)
@@ -44,7 +43,7 @@ compare out out_ok || fail=1
LS_COLORS="mh=$code_mh" ls -U1 --color=always file1 file2 > out || fail=1
printf "$c0${c_mh}file1$c0
${c_mh}file2$c0
-$c_end" > out_ok || framework_failure
+" > out_ok || framework_failure
compare out out_ok || fail=1
# hard links and png (hard link coloring takes precedence)
@@ -53,7 +52,7 @@ LS_COLORS="mh=$code_mh:*.png=$code_png" ls -U1 --color=always
file1 file2.png \
> out || fail=1
printf "$c0${c_mh}file1$c0
${c_mh}file2.png$c0
-$c_end" > out_ok || framework_failure
+" > out_ok || framework_failure
compare out out_ok || fail=1
# hard links and exe (exe coloring takes precedence)
@@ -63,7 +62,7 @@ LS_COLORS="mh=$code_mh:*.png=$code_png:ex=$code_ex" \
chmod a-x file2.png || framework_f
Re: [PATCH] Fix exit status of signal handlers in shell scripts
On Sat, Jan 30, 2010 at 12:28:50PM -0700, Eric Blake wrote: > According to Dmitry V. Levin on 1/30/2010 12:18 PM: > > The value of `$?' on entrance to signal handlers in shell scripts > > cannot be relied upon, so set the exit code explicitly to > > 128 + SIGTERM == 143. > > * src/Makefile.am (sc_tight_scope): Use `exit 143' in signal handler. > > I'm not sure I like the direction this is headed in. Exiting with 143 > when a trap is known to be caused by SIGTERM might be okay, but it would > be even better to reraise the signal and make the shell also exit by > SIGTERM (in case the caller can distinguish between exit by signal and > normal exit by status > 128). But blindly giving status 143 for other > signals, like SIGHUP, is just wrong. If you are going to munge trap > handlers to account for races, then you need one trap handler per signal > with an appropriate exit status for each. One trap handler per signal is overkill in most cases. I think that any non-zero exit status would be sufficient. -- ldv pgpkkCBUeckkB.pgp Description: PGP signature
Re: [PATCH] Fix exit status of signal handlers in shell scripts
According to Dmitry V. Levin on 1/30/2010 12:18 PM: > The value of `$?' on entrance to signal handlers in shell scripts > cannot be relied upon, so set the exit code explicitly to > 128 + SIGTERM == 143. > * src/Makefile.am (sc_tight_scope): Use `exit 143' in signal handler. I'm not sure I like the direction this is headed in. Exiting with 143 when a trap is known to be caused by SIGTERM might be okay, but it would be even better to reraise the signal and make the shell also exit by SIGTERM (in case the caller can distinguish between exit by signal and normal exit by status > 128). But blindly giving status 143 for other signals, like SIGHUP, is just wrong. If you are going to munge trap handlers to account for races, then you need one trap handler per signal with an appropriate exit status for each. -- Don't work too hard, make some time for fun as well! Eric Blake e...@byu.net signature.asc Description: OpenPGP digital signature
[PATCH] Fix exit status of signal handlers in shell scripts
The value of `$?' on entrance to signal handlers in shell scripts
cannot be relied upon, so set the exit code explicitly to
128 + SIGTERM == 143.
* src/Makefile.am (sc_tight_scope): Use `exit 143' in signal handler.
* cfg.mk (sc_always_defined_macros, sc_system_h_headers): Likewise.
* tests/test-lib.sh: Use `Exit 143' in signal handler.
---
See also
http://lists.gnu.org/archive/html/bug-gnulib/2010-01/msg00361.html
cfg.mk|6 --
src/Makefile.am |3 ++-
tests/test-lib.sh |2 +-
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index b5a21c3..52e78ae 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -130,7 +130,8 @@ headers_with_interesting_macro_defs = \
# Don't define macros that we already get from gnulib header files.
sc_always_defined_macros: .re-defmac
@if test -f $(srcdir)/src/system.h; then\
- trap 'rc=$$?; rm -f .re-defmac; exit $$rc' 0 1 2 3 15;\
+ trap 'rc=$$?; rm -f .re-defmac; exit $$rc' 0; \
+ trap 'exit 143' 1 2 3 15; \
grep -f .re-defmac $$($(VC_LIST)) \
&& { echo '$(ME): define the above via some gnulib .h file' \
1>&2; exit 1; } || :;\
@@ -149,7 +150,8 @@ sc_always_defined_macros: .re-defmac
# the headers already included via system.h.
sc_system_h_headers: .re-list
@if test -f $(srcdir)/src/system.h; then\
- trap 'rc=$$?; rm -f .re-list; exit $$rc' 0 1 2 3 15; \
+ trap 'rc=$$?; rm -f .re-list; exit $$rc' 0; \
+ trap 'exit 143' 1 2 3 15; \
grep -nE -f .re-list \
$$($(VC_LIST_EXCEPT) | grep '^src/') \
&& { echo '$(ME): the above are already included via system.h'\
diff --git a/src/Makefile.am b/src/Makefile.am
index c73f7dc..90e3b3e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -709,7 +709,8 @@ sc_check-AUTHORS: $(all_programs)
.PHONY: sc_tight_scope
sc_tight_scope: $(bin_PROGRAMS)
$(AM_V_GEN)t=exceptions-; \
- trap "s=$$?; rm -f $$t; exit $$s" 0 1 2 13 15; \
+ trap "s=$$?; rm -f $$t; exit $$s" 0;\
+ trap "exit 143" 1 2 13 15; \
src=`for f in $(SOURCES); do\
test -f $$f && d= || d=$(srcdir)/; echo $$d$$f; done`; \
hdr=`for f in $(noinst_HEADERS); do \
diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index 7ad4331..81950e2 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -408,7 +408,7 @@ remove_tmp_()
# Run each test from within a temporary sub-directory named after the
# test itself, and arrange to remove it upon exception or normal exit.
trap remove_tmp_ 0
-trap 'Exit $?' 1 2 13 15
+trap 'Exit 143' 1 2 13 15
cd "$t_" || error_ "failed to cd to $t_"
--
ldv
Re: modify chmod
Jim Meyering writes: > This new behavior could be useful... > maybe noticeably more efficient in some cases, too. > Perhaps worth a new option, if not the default. In rereading the old bug report, I noticed that FreeBSD is listed as the only OS that does things the more-efficient way. I looked at the FreeBSD 8.0 chmod.c source, and what it actually does is apply the optimization only if it knows that the file system does not have ACLs (it calls something named "may_have_nfs4acl", because apparently NFSv4 ACLs might change even if you apply a mode that's the same as before). I suggest that we do something similar.
Re: modify chmod
According to Jim Meyering on 1/30/2010 8:50 AM:
>> Does it make sense to make chmod do not change the inode's ctime
>> if the new permission bits is identical to the old as the default behavior?
I still think POSIX allows it. Rather than calling out chmod(2) behavior,
POSIX states that for chmod(1):
"Upon successfully changing the file mode bits of a file, the chmod
utility shall mark for update the last file status change timestamp of the
file."
> With the patch below, it skips the chmodat call altogether, because
> chmod notices that "u+x" would not modify the existing permissions:
>
> $ ./chmod u+x /
> $
>
> If we were to insist that the modified chmod
> produce the same diagnostic, we *could* try.
> But we'd have to compare effective uid to stat.st_uid, ...
> and what about the possibility of ACLs? So I think that is
> not an option.
>
> This new behavior could be useful...
> maybe noticeably more efficient in some cases, too.
> Perhaps worth a new option, if not the default.
I think that optimizing no-op chmod(1) by default is reasonable, but it
would still be nice to have an option to guarantee a chmod(2) call (for
its ACL side-effects, even if the permission bits appear to be a no-op).
> - if (! S_ISLNK (old_mode))
> + if ((old_mode & CHMOD_MODE_BITS) == new_mode)
One other thing - I'm still hoping to find some time to work on:
providing a gnulib replacement to work around Solaris 9's bug ('chmod
file/' mistakenly succeeds)
teaching coreutils chmod to use lchmod (and/or
fchmodat(,AT_SYMLINK_NOFOLLOW)) on systems that support it (BSD systems at
the moment), such that we can copy BSD's 'chmod -h' and 'chmod -PR'.
Such a patch would impact the same areas of code as the patch under
discussion in this thread.
--
Don't work too hard, make some time for fun as well!
Eric Blake e...@byu.net
signature.asc
Description: OpenPGP digital signature
Re: modify chmod
jeff.liu wrote:
> It looks the "Modify chmod" task has been in TODO list for a long time.
>
> I have looked through the discussion thread referred to
> http://bugs.debian.org/497514.
>
> Does it make sense to make chmod do not change the inode's ctime
> if the new permission bits is identical to the old as the default behavior?
>
> I'd like to handle it if it is still a valid task and nobody is working
> on for now.
Hi Jeff,
Thanks for bringing that up.
I refreshed my memory, wrote a tiny patch
and tested the result. Running as non-root,
must "chmod u+x /" fail? With coreutils-8.4, it does:
$ chmod u+x /
chmod: changing permissions of `/': Operation not permitted
With the patch below, it skips the chmodat call altogether, because
chmod notices that "u+x" would not modify the existing permissions:
$ ./chmod u+x /
$
If we were to insist that the modified chmod
produce the same diagnostic, we *could* try.
But we'd have to compare effective uid to stat.st_uid, ...
and what about the possibility of ACLs? So I think that is
not an option.
This new behavior could be useful...
maybe noticeably more efficient in some cases, too.
Perhaps worth a new option, if not the default.
Opinions?
diff --git a/src/chmod.c b/src/chmod.c
index 3dab92e..838ba2f 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -258,16 +258,24 @@ process_file (FTS *fts, FTSENT *ent)
new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
change, NULL);
- if (! S_ISLNK (old_mode))
+ if ((old_mode & CHMOD_MODE_BITS) == new_mode)
{
- if (chmodat (fts->fts_cwd_fd, file, new_mode) == 0)
-chmod_succeeded = true;
- else
+ /* Perm+special bits are the same, so skip the chmod altogether. */
+ chmod_succeeded = true;
+}
+ else
+{
+ if (! S_ISLNK (old_mode))
{
- if (! force_silent)
-error (0, errno, _("changing permissions of %s"),
- quote (file_full_name));
- ok = false;
+ if (chmodat (fts->fts_cwd_fd, file, new_mode) == 0)
+chmod_succeeded = true;
+ else
+{
+ if (! force_silent)
+error (0, errno, _("changing permissions of %s"),
+ quote (file_full_name));
+ ok = false;
+}
}
}
}
