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
