Re: [PATCH] Again, do not change the mode of all directories below $HOME.

[Jim Meyering]

Ralf Wildenhues <address@hidden> wrote:
>> 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.

Hi Ralf,

I wouldn't call it admitting defeat.
It's more like admitting that not everyone will go to
the required lengths to make their code robust enough
to deal with such situations.

>> 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

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.

> 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.

> 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.

Patches welcome.