Re: svn commit: r1803722 - /subversion/branches/1.9.x/STATUS

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803722 - /subversion/branches/1.9.x/STATUS

Daniel Shahaf-2
[hidden email] wrote on Wed, Aug 02, 2017 at 01:35:31 -0000:
>   * r1802032
>     Install 'fsfs-stats' as a wrapper to 'svnfsfs', to which it was renamed in
>     r1618848.
>     Justification:
>       Backwards compatibility with 1.8.x tools/.
>     Votes:
> +     -0: jamessan ($(bindir) and $$1 should be quoted in case they contain shell metacharacters)

Thanks for the review.  I'll fix $1 in a moment, but why does $(bindir)
need to be quoted?  The makefiles use $(bindir) unquoted [1],
so I assumed what was safe for shell commands in Makefile is safe for
shell commands on the installed system.

Or do you just mean changing:
   printf %s 'foo $(bindir) bar'
to:
   printf 'foo %s bar' $(bindir)
?

Cheers,

Daniel

[1]
% grep -h -w bindir ${abs_srcdir}/build-outputs.mk ./Makefile
        $(MKDIR) $(DESTDIR)$(bindir)
        cd subversion/svn ; $(INSTALL_BIN) svn$(EXEEXT) $(DESTDIR)$(bindir)/svn$(EXEEXT)
        cd subversion/svnadmin ; $(INSTALL_BIN) svnadmin$(EXEEXT) $(DESTDIR)$(bindir)/svnadmin$(EXEEXT)
        cd subversion/svnbench ; $(INSTALL_BIN) svnbench$(EXEEXT) $(DESTDIR)$(bindir)/svnbench$(EXEEXT)
        cd subversion/svndumpfilter ; $(INSTALL_BIN) svndumpfilter$(EXEEXT) $(DESTDIR)$(bindir)/svndumpfilter$(EXEEXT)
        cd subversion/svnfsfs ; $(INSTALL_BIN) svnfsfs$(EXEEXT) $(DESTDIR)$(bindir)/svnfsfs$(EXEEXT)
        cd subversion/svnlook ; $(INSTALL_BIN) svnlook$(EXEEXT) $(DESTDIR)$(bindir)/svnlook$(EXEEXT)
        cd subversion/svnmucc ; $(INSTALL_BIN) svnmucc$(EXEEXT) $(DESTDIR)$(bindir)/svnmucc$(EXEEXT)
        cd subversion/svnrdump ; $(INSTALL_BIN) svnrdump$(EXEEXT) $(DESTDIR)$(bindir)/svnrdump$(EXEEXT)
        cd subversion/svnserve ; $(INSTALL_BIN) svnserve$(EXEEXT) $(DESTDIR)$(bindir)/svnserve$(EXEEXT)
        cd subversion/svnsync ; $(INSTALL_BIN) svnsync$(EXEEXT) $(DESTDIR)$(bindir)/svnsync$(EXEEXT)
        cd subversion/svnversion ; $(INSTALL_BIN) svnversion$(EXEEXT) $(DESTDIR)$(bindir)/svnversion$(EXEEXT)
bindir = ${exec_prefix}/bin
  $(MKDIR) $(DESTDIR)$(bindir); \
  ln -sf svnmucc$(EXEEXT) $(DESTDIR)$(bindir)/svnsyitf$(EXEEXT); \
  if test "$(DESTDIR)$(bindir)" != "$(DESTDIR)$(toolsdir)"; then \
    ln -sf $(bindir)/svnmucc$(EXEEXT) $(DESTDIR)$(toolsdir)/svnmucc$(EXEEXT); \
    ln -sf $(bindir)/svnbench$(EXEEXT) $(DESTDIR)$(toolsdir)/svn-bench$(EXEEXT); \
    'exec $(bindir)/svnfsfs stats $${2:+"-M"} $$2 $$1' \
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803722 - /subversion/branches/1.9.x/STATUS

James McCoy-4
On Wed, Aug 02, 2017 at 02:07:20PM +0000, Daniel Shahaf wrote:

> [hidden email] wrote on Wed, Aug 02, 2017 at 01:35:31 -0000:
> >   * r1802032
> >     Install 'fsfs-stats' as a wrapper to 'svnfsfs', to which it was renamed in
> >     r1618848.
> >     Justification:
> >       Backwards compatibility with 1.8.x tools/.
> >     Votes:
> > +     -0: jamessan ($(bindir) and $$1 should be quoted in case they contain shell metacharacters)
>
> Thanks for the review.  I'll fix $1 in a moment, but why does $(bindir)
> need to be quoted?  The makefiles use $(bindir) unquoted [1],
> so I assumed what was safe for shell commands in Makefile is safe for
> shell commands on the installed system.

Just because no one has run into trouble with the existing code, doesn't
mean it's right. :) Sure, $(bindir) is _typically_ going to be safe to
use unquoted, but why rely on that?  If I run "./configure --prefix
'/home/jamessan/things & stuff'" and then "make", the Makefile breaks
horribly.  I don't even need to try installing.  It fails while creating
the .la files.

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7  2D23 DFE6 91AE 331B A3DB
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1803722 - /subversion/branches/1.9.x/STATUS

Daniel Shahaf-2
James McCoy wrote on Wed, Aug 02, 2017 at 20:51:20 -0400:

> On Wed, Aug 02, 2017 at 02:07:20PM +0000, Daniel Shahaf wrote:
> > [hidden email] wrote on Wed, Aug 02, 2017 at 01:35:31 -0000:
> > >   * r1802032
> > >     Install 'fsfs-stats' as a wrapper to 'svnfsfs', to which it was renamed in
> > >     r1618848.
> > >     Justification:
> > >       Backwards compatibility with 1.8.x tools/.
> > >     Votes:
> > > +     -0: jamessan ($(bindir) and $$1 should be quoted in case they contain shell metacharacters)
> >
> > Thanks for the review.  I'll fix $1 in a moment, but why does $(bindir)
> > need to be quoted?  The makefiles use $(bindir) unquoted [1],
> > so I assumed what was safe for shell commands in Makefile is safe for
> > shell commands on the installed system.
>
> Just because no one has run into trouble with the existing code, doesn't
> mean it's right. :) Sure, $(bindir) is _typically_ going to be safe to
> use unquoted, but why rely on that?

For consistency with every other use of «$(bindir)» in Makefile.

If someone actually wanted to install to a directory with spaces today,
they'd probably invoke configure as «./configure --prefix=/foo\\\ bar»,
or change «prefix = /foo bar» to «prefix = "/foo bar"» in Makefile after
running configure.  That would cause all uses of «$(bindir)» unquoted to
work, but would break wherever «"$(bindir)"» double-quoted is used.

So perhaps we should quote _all_ uses of «$(bindir)»; but not just one.

Makes sense?

Cheers,

Daniel

> If I run "./configure --prefix
> '/home/jamessan/things & stuff'" and then "make", the Makefile breaks
> horribly.  I don't even need to try installing.  It fails while creating
> the .la files.
Loading...