Hi Eric,
* Eric Blake wrote on Mon, Nov 17, 2008 at 12:49:25AM CET:
> According to Ralf Wildenhues on 11/16/2008 3:48 PM:
> > - my_directory_path=`$ECHO "X$my_directory_path" | $Xsed -e
> > "$dirname"`
> > + my_directory_path=`$ECHO "$my_directory_path" | $SED -e "$dirname"`
> > done
> > - my_dir_list=`$ECHO "X$my_dir_list" | $Xsed -e 's,:*$,,'`
> > + my_dir_list=`$ECHO "$my_dir_list" | $SED 's,:*$,,'`
>
> You generally removed the -e (which is a good move in my mind), but
> weren't consistent about it.
Yeah, I wasn't. In some places it felt awkward or isn't portable, in
some I simply forgot.
> > $ECHO "run \`$progname --help | more' for full usage"
>
> This can be echo rather than $ECHO, since the \` is flattened during ""
> interpratation prior to the echo call. Several similar places exist.
What about $progname? Ah, it should be a basename. Well, our current
basename functions don't compute the basename of files with backslash
as directory separator.
> > - $ECHO "host: $host"
> > + echo "host: $host"
>
> Are we guaranteed that $host never contains \?
Yes, looking at config.guess and config.sub, I think so. I'd anyway
consider it a bug in those scripts if they output something not in the
portable file name set of characters: it should be possible to use
mkdir build/`config.guess`
> > - $ECHO "export $shlibpath_var"
> > + echo "export $shlibpath_var"
>
> Likewise, for shlibpath_var.
Yes, it contains the name of a shell variable.
> > - $ECHO >> "$output_objdir/$my_dlsyms" "\
> > + echo >> "$output_objdir/$my_dlsyms" "\
> >
> > /* The mapping between symbol names and symbols. */
> > typedef struct {
> > @@ -2030,7 +2030,7 @@ typedef struct {
>
> This lacks context to see if it is safe, or if the text being appended
> contains \. Several instance of this idiom.
I generally checked these.
> > @@ -2364,13 +2363,13 @@ _LTECHO_EOF'
> > fi\
> >
> > # Find the directory that this script lives in.
> > - thisdir=\`\$ECHO \"X\$file\" | \$Xsed -e 's%/[^/]*$%%'\`
> > + thisdir=\`\$ECHO \"\$file\" | $SED 's%/[^/]*$%%'\`
>
> Similar to Paolo's patch, my comment still holds that this area of code
> would be much more maintainable with an AS_ESCAPE (or a similar
> m4_bpatsubst, since AS_ESCAPE is not yet a documented m4sh interface),
> rather than hand-escaping the contents of here-doc's and eval's. But that
> can (should) be a separate patch.
Exactly.
> Meanwhile, did you really mean $SED, or did you want \$SED?
Well, $SED is not initialized in the sub script, so yes, I meant that.
I guess initializing it would be cleaner, though.
> > $ECHO "*** because the file extensions .$libext of this
> > argument makes me believe"
> Is $libext ever likely to contain \, or can we use plain echo here, too?
\ would be unlikely, I guess.
> > - output_la=`$ECHO "X$output" | $Xsed -e "$basename"`
> > + output_la=`$ECHO "$output" | $SED "$basename"`
>
> Wouldn't func_basename be better?
Yes, definitely; I intended to fix some of these instances in a separate
patch.
> > -[$1='`$ECHO "X$][$1" | $Xsed -e "$delay_single_quote_subst"`'])
> > +[$1='`$ECHO "$][$1" | $SED "$delay_single_quote_subst"`'])
>
> Unrelated to your patch, but this does not need ][ in the middle. M4
> outputs $$1 as a literal $ followed by the first argument, without needing
> separation between the quoted strings.
Ah, thanks.
> > -# <var>='`$ECHO "X$<var>" | $Xsed -e "$delay_single_quote_subst"`'
> > +# <var>='`$ECHO "$<var>" | $$SED "$delay_single_quote_subst"`'
>
> Typo in this comment?
Looks like it. Thanks!
> > # If test is not a shell built-in, we'll probably end up computing a
> > # maximum length that is only half of the actual maximum length, but
> > # we can't tell.
> > - while { test "X"`$ECHO "X$teststring$teststring" 2>/dev/null` \
> > - = "XX$teststring$teststring"; } >/dev/null 2>&1 &&
> > + while { test "X"`$ECHO "$teststring$teststring" 2>/dev/null` \
> > + = "X$teststring$teststring"; } >/dev/null 2>&1 &&
>
> Does $teststring contain \, or can we use echo here to avoid forks in this
> loop on shells where $ECHO is expensive?
Well. This code is meant to find out the maximum command line length
limit. It is supposed to fork and exec. Thinking about it, I wonder.
It will likely fork with most shells (but not all), but chances are not
that high that it will exec. Means the test looks broken anyway. :-/
> > - RM="$ECHO $RM"
> > - test -n "$LN_S" && LN_S="$ECHO $LN_S"
> > - CP="$ECHO $CP"
> > - MKDIR="$ECHO $MKDIR"
> > - TAR="$ECHO $TAR"
> > + RM="echo $RM"
> > + test -n "$LN_S" && LN_S="echo $LN_S"
> > + CP="echo $CP"
> > + MKDIR="echo $MKDIR"
> > + TAR="echo $TAR"
>
> Are all of these safe, considering mingw might have \ in an absolute
> pathname to these tools?
No, and I'm glad you ask about this. The point here is that there is
more than one argument following, and I didn't see an easy way to escape
them properly, so I figured take the less likely of the bugs, as
RM="$ECHO rm -f" is quite certain to do the wrong thing. Suggestions?
Thanks!
Ralf
