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

[Ralf Wildenhues]

Hi Jim, and sorry for the delay,

* Jim Meyering wrote on Tue, Jul 22, 2008 at 09:42:30PM CEST:
> Ralf Wildenhues <address@hidden> wrote:
> >
> >> 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 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.

Granted.  But please don't disallow white space in the build dir for a
normal "./configure && make all install".  That would hurt some users.

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

Hmm.  I think I fail to see what exactly the shell-under-quoting was
then.  Could you be bothered to explain?  Thanks.

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

Cheers,
Ralf