Re: [PATCH 1/3] Add the ``--mount'' command line option

[Thomas Schwinge]

Hello!

On Fri, Jun 12, 2009 at 04:40:12PM +0300, Sergiu Ivanov wrote:
> On Fri, Jun 12, 2009 at 08:53:50AM +0200, Carl Fredrik Hammar wrote:
> > On Thu, Jun 11, 2009 at 09:10:24PM +0300, Sergiu Ivanov wrote:
> > > >From d0f0f5c41d9046aec765a7264914c19642adead9 Mon Sep 17 00:00:00 2001
> > > From: Sergiu Ivanov <unlimitedscolobb@gmail.com>
> > > Date: Thu, 11 Jun 2009 15:22:24 +0300
> > > Subject: [PATCH] Add the ``--mount'' command line option.
> > > 
> > > +++ b/unionmount.c
> > > @@ -0,0 +1,28 @@
> > > +/* Hurd unionmount
> > > +   The core of unionmount functionality.
> > > +
> > > +   Copyright (C) 2009 Free Software Foundation, Inc.
> > > +
> > > +   Written by Sergiu Ivanov <unlimitedscolobb@gmail.com>.
> > > +
> > > +   This program is free software; you can redistribute it and/or
> > > +   modify it under the terms of the GNU General Public License version
> > > +   2 as published by the Free Software Foundation.
> > 
> > This should be:
> > 
> >    This program is free software; you can redistribute it and/or
> >    modify it under the terms of the GNU General Public License
> >    as published by the Free Software Foundation; either version 2
> >    of the License, or (at your option) any later version.
> > 
> > Note the ``or any later version'' part.
> 
> Yes, it initially was like that, but Thomas told me that the idea
> about later versions is implied... (or, actually, it was how I
> understood his remark about the ``implied *'').

Ah, what I mean was simply that there was a literal asterisk character
inside the text.  (See my Makefile patch posted earlier where I removed
it.)


> > > +   You should have received a copy of the GNU General Public License
> > > +   along with this program; if not, write to the Free Software
> > > +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> > > +   USA.  */
> > 
> > I remember being told that the address has changed when I was working
> > on libchannel.  And indeed the address is different in my libchannel
> > source:
> > 
> >    If not, write to the Free Software Foundation, 675 Mass Ave, Cambridge,
> >    MA 02139, USA.
> > 
> > I think your using the old address but I'm not sure.  Thomas will have
> > to clarify.  Perhaps there is a definitive source somewhere.
> 
> Hm, nice... I've never heard of that change... I'll wait for what
> Thomas should say, if you don't mind...

Sure there is a definitive source:
<http://www.fsf.org/about/contact.html>.  So, both of you are wrong.  ;-P

Obviously, use the current address when creating new files, and then we
can sometime later bother to update all the other files in one go.  This
also need to be adjusted in the main Hurd sources, etc.


On Sat, Jun 13, 2009 at 03:53:27PM +0200, Carl Fredrik Hammar wrote:
> This time around I'll review your patch more thoroughly.

Thanks for your help, by the way!  Unless I'm noting specifically, I'm
fine with your comments.

> On Thu, Jun 11, 2009 at 09:10:24PM +0300, Sergiu Ivanov wrote:
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1,6 +1,10 @@
> >  # Hurd unionfs
> > -# Copyright (C) 2001, 2002, 2003, 2005 Free Software Foundation, Inc.
> > +# Copyright (C) 2001, 2002, 2003, 2005, 2009 Free Software Foundation,
> > +# Inc.
> 
> Break the line after the years instead, like so:
> 
> # Copyright (C) 2001, 2002, 2003, 2005, 2009
> #   Free Software Foundation, Inc.
> 
> As is done in other parts of the Hurd.

Also note that this is not done very consistently.  Some months ago, I
switched to simply have Emacs' M-q (fill-paragraph) do this work --
that's why I'm adding blank lines before and after the Copyright lines
when amending them (if they're not there already).

> > --- a/options.c
> > +++ b/options.c
> > @@ -51,6 +56,8 @@ static const struct argp_option argp_common_options[] =
> >        "send debugging messages to stderr" },
> >      { OPT_LONG_CACHE_SIZE, OPT_CACHE_SIZE, "SIZE", 0,
> >        "specify the maximum number of nodes in the cache" },
> > +    { OPT_LONG_MOUNT, OPT_MOUNT, "MOUNTEE", 0,
> > +      "start MOUNTEE and add the filesystem it publishes" },
> 
> "start the translator MOUNTEE and add it's filesystem" would be clearer
> IMHO.

I think it would also help if this comment made it clear that this is a
whole *translator command line* that is to be specified there, as
compared to only the translator binary.


Back to your latest version of the patch:

On Mon, Jun 15, 2009 at 10:39:22PM +0300, Sergiu Ivanov wrote:
> From 87963e32b39b8bc8f6a29f5033d99e7f0583c5b4 Mon Sep 17 00:00:00 2001
> From: Sergiu Ivanov <unlimitedscolobb@gmail.com>
> Date: Thu, 11 Jun 2009 15:22:24 +0300
> Subject: [PATCH] Add the ``--mount'' command line option.
> 
> * Makefile: [...]
> Update copyright information.

No need to reflect such non-functional changes in the ChangeLog / commit
message.

> * options.h (OPT_MOUNT): Add the definition.
> (OPT_LONG_MOUNT): Likewise.
> Update copyright information.
> 
> * options.c (argp_common_options): Add option ``--mount''

There is no need to put blank lines between changed files that are
related to each other in the current changeset.

Please don't forget to mention new files.

So, instead of your version...

    * Makefile: Add unionmount.c to the list of compiled
    object files.
    Update copyright information.
    
    * options.h (OPT_MOUNT): Add the definition.
    (OPT_LONG_MOUNT): Likewise.
    Update copyright information.
    
    * options.c (argp_common_options): Add option ``--mount''
    to the array of options.
    (argp_parse_common_options): Add the code for handling option
    ``--mount''.
    Update copyright information.

... I would have written this commit message as follows:

    * Makefile (OBJS): Add unionmount.o.
    * options.h (OPT_MOUNT, OPT_LONG_MOUNT): Define.
    * options.c (argp_common_options): Add option ``--mount''.
    (argp_parse_common_options): Handle it.
    * unionmount.h: New file.
    * unionmount.c: Likewise.

> --- a/options.c
> +++ b/options.c
> @@ -124,6 +131,13 @@ argp_parse_common_options (int key, char *arg, struct 
> argp_state *state)
>        ulfs_match = 0;
>        break;
>  
> +    case OPT_MOUNT:
> +      /* TODO: Improve the mountee command line parsing mechanism.  */
> +      err = argz_create_sep (arg, ' ', &mountee_argz, &mountee_argz_len);

If I understand it correctly, indeed: doing it this way, you loose the
ability to pass strings containing ``quoted space characters'', i.e. ones
that do not separate arguments.  Of couse, for simple use cases it
doesn't matter too much.

If only one --mount option is allowed, what about a syntax like this one,
separating the mountee command line with two dashes?

    unionfs [OPTION...] --mount [FILESYSTEMS...] -- MOUNTEE_CMD_LINE

... and an explanation like ``--mount: if specified start
MOUNTEE_CMD_LINE and add [...]''.  But no, I don't really like that
either.  Hmm...

There must be other applications that have the same problem --
unfortunately it's too late for me to investigate and see how they solve
this quoting / argument splitting problem.


> diff --git a/unionmount.c b/unionmount.c
> new file mode 100644
> index 0000000..2b3041f
> --- /dev/null
> +++ b/unionmount.c

> +#define _GNU_SOURCE 1

That should usually be done in the Makefile / build system.


Regards,
 Thomas

signature.asc
Description: Digital signature