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

[Sergiu Ivanov]

Hello,

On Wed, Jun 17, 2009 at 01:43:42AM +0200, Thomas Schwinge wrote:
> On Sat, Jun 13, 2009 at 03:53:27PM +0200, Carl Fredrik Hammar wrote:
> > 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).

Yeah, I was doing it in the same way, that's why I had the line broken
before Inc. in the first version of the patch. However, I guess I'll
keep it like Carl said.

> Back to your latest version of the patch:
> 
> On Mon, Jun 15, 2009 at 10:39:22PM +0300, Sergiu Ivanov wrote:
> > * 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.

I'm sorry, but could you please give an example where there *is* a
need for a blank line? (I just feel a bit dizzy about that changeset
terminology :-( )
 
> So, instead of your version...
> 
>     [...]
>     (argp_parse_common_options): Add the code for handling option
>     ``--mount''.
>     [...]
>
> ... I would have written this commit message as follows:
>
>     [...]
>     (argp_parse_common_options): Handle it.
>     [...]

I took the liberty of making this changelog entry a bit longer
(``Handle the new option''). The reason is that it took me some time
to realize what ``it'' refers to. I hope this deviation isn't that
critical...

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

In this patch series I do pursue simple use-cases :-)
 
> 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...

The idea of this patch series is to implement the union mount
functionality in a basic (proof-of-concept) way. Note that it's not
absolutely necessary that the unionmount project will preserve its
future status of a unionfs option; the whole ``--mount'' stuff is
likely to fall to oblivion in such a case.
 
> 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.

I'll try to make an investigation of my own, shall we choose the
continue the ``--mount'' idea, but I'll definitely be happy to have
your help in that situation :-)
 
> > 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.

All .c files in unionfs define _GNU_SOURCE explicitly (though, true,
they do only #define _GNU_SOURCE /*(no 1)*/, and I've changed my new
patch accordingly). I chose to do similarly in unionmount.c, because
moving the definition of _GNU_SOURCE into Makefile (if I understand
you correctly) will mean changing a lot stuff loosely connected to
unionmount matters. I guess this transition has its place in a
separate clean-up patch.

Regards,
scolobb