[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: |
Jim Meyering |
Subject: |
Re: [PATCH] Again, do not change the mode of all directories below $HOME. |
Date: |
Sat, 02 Aug 2008 12:43:56 +0200 |
Ralf Wildenhues <address@hidden> wrote:
...
>> > 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
>>
>> I don't see how that would change anything. The cd is just to
>> get out of the directory we're about to remove. It worked.
>> The chmod is what failed, operating on the prefix up to,
>> but not including the space.
>
> Hmm. I think I fail to see what exactly the shell-under-quoting was
> then. Could you be bothered to explain? Thanks.
>From memory, ...
when building with a space-polluted build and/or source directory name,
there was at least one failure (or mere diagnostic leading to a directory
not being removed?) that I tracked down to the failure of that chmod
command. The chmod diagnostic mentioned the prefix before the space.
>> > 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?
>>
>> I'd be more amenable to a patch that makes a macro like
>> posix-shell.m4 choose a shell with the features I use.
>> Since no one has reported trouble yet, perhaps it's not a problem
>> in practice.
>
> Hmm. Maybe you're right. That would be very cool to have 'local'
> available.
>
>> > 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 ...
>>
>> Yes. That was deliberate.
>> With those rules, I have never bothered to accommodate white space in TMPDIR.
>> I expect that anyone clueful enough to run those optional rules also
>> knows not to choose a TMPDIR value that is likely to cause trouble.
>
> That is fine with me. I just thought I'd mention it.
Hmm... don't want to give the wrong impression.
I don't want to obfuscate the code to accommodate a pathological TMPDIR
value. However, it'd be far better to make things fail with a sensible
diagnostic when TMPDIR has an invalid value, than to continue on
and misbehave.
too little time...