[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Again, do not change the mode of all directories below $HOME
From: |
Ralf Wildenhues |
Subject: |
Re: [PATCH] Again, do not change the mode of all directories below $HOME. |
Date: |
Tue, 22 Jul 2008 20:02:28 +0200 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
Hi Jim,
* Jim Meyering wrote on Tue, Jul 22, 2008 at 01:17:20PM CEST:
> Ralf Wildenhues <address@hidden> wrote:
> > this fixes a regression of "make check" when source and build tree live
> > in a directory with spaces in the name. The regression was introduced
> > some time after I posted a fix for the issue last time.
> Look at it as an incentive not to use directories with names
> containing meta-characters ;-)
Well, *I* am doing it for fun. Others use /c/My Files/... because yet
others thought requiring users to cope with such names would be a cool
idea. The slightly provoking subject is to remind you that I suffered
when this happened to me for the first time. ;-)
> But seriously, considering the potential for damage and mischief,
> perhaps it's time to add infrastructure that stops e.g., configure
> in its tracks whenever it detects a potentially troublesome source or
> build dir.
I disagree. You can do that and admit defeat. I worked to mostly[1]
fix Automake for this, because users kept complaining, and now refuse
to admit defeat.
> I applied your patch, then added a test to ensure there is no further
> regression, in spite of the added cost to "make distcheck". Finally,
> I fixed a similar (and long-standing!) shell-under-quoting bug in
> test-lib.sh that was exposed by the cleanup (trap-run) code not removing
> a per-test directory for the tests that create unsearchable directories.
> With a bad build directory containing an "a b" component, instead of
> running e.g., chmod -R 700 "/.../a b/...", the pre-patch trap/cleanup
> code would run chmod -R 700 "/.../a".
Would the following not have sufficed, too?
> -trap 'st=$?; cleanup_; d='"$t_"';
> - cd '"$test_dir_"' && chmod -R u+rwx "$d" && rm -rf "$d" && exit $st' 0
+trap 'st=$?; cleanup_; d="$t_";
+ cd "$test_dir_" && chmod -R u+rwx "$d" && rm -rf "$d" && exit $st' 0
BTW, I see that you use 'local' in shell functions. This isn't required
to be supported by POSIX sh, and AFAIK won't work with some ksh variants,
for example the "Version M 1993-12-28 r" on my GNU/Linux system, but
also your test-lib.sh doesn't check for this capability. I suggest to
just avoid local variables, since you have only a couple anyway. Should
I write a patch to this end?
FWIW, your second patch looks like it won't quite do what you intended
if TMPDIR contains white space:
tp := $(shell echo "$(TMPDIR)/$(PACKAGE)-$$$$")
t_prefix = $(tp)/a
... using $(t_prefix) unquoted in a number of places ...
Cheers,
Ralf
[1] You currently have to do something like this:
./configure -C MISSING=/usr/share/automake-1.10a/missing \
install_sh=/usr/share/automake-1.10a/install-sh