bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 5/5] Changed argp parsing policy


From: Thomas Schwinge
Subject: Re: [PATCH 5/5] Changed argp parsing policy
Date: Wed, 27 May 2009 22:27:33 +0200
User-agent: Mutt/1.5.11

Hello!

On Tue, May 26, 2009 at 11:31:59PM +0300, Sergiu Ivanov wrote:
> diff --git a/Makefile b/Makefile
> index b7e5716..7b7ce01 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -21,7 +21,7 @@
>  # USA.
> 
>  # Get the information from under /usr.
> -prefix = /usr/
> +prefix = /usr

Aha, here we go ;-).  But please put it like this into your first commit.

> -OBJS = main.o node.o lnode.o ulfs.o ncache.o netfs.o \
> -       lib.o options.o pattern.o stow.o update.o
> +OBJS = main.o node.o lnode.o ulfs.o ncache.o netfs.o lib.o options.o \
> +       pattern.o stow.o update.o unionmount.o

This is not really that important here (and the same applies to my other
nit-picking by the way -- but you chose to be with those picky Hurd
people, didn't you?), but as a guideline for forthcoming situations, and
to make reviewing your changes easier: try to preserve as much as is
possible from the previous version.  (As long as is sensible, of course.)

Make this change look like this:

>  OBJS = main.o node.o lnode.o ulfs.o ncache.o netfs.o \
> -       lib.o options.o pattern.o stow.o update.o
> +       lib.o options.o pattern.o stow.o update.o unionmount.o

Or like this:

>  OBJS = main.o node.o lnode.o ulfs.o ncache.o netfs.o \
> -       lib.o options.o pattern.o stow.o update.o
> +       lib.o options.o pattern.o stow.o update.o \
> +       unionmount.o

That way it is easier for the reviewer to grasp that you simply added
unionmount.o and didn't do any other changes to the list.

Of course, it is also possible to introduce a commit like ``Reformat OBJS
for better readability'', or ``... for better use of display width'', or
whatever, before doing your change.  And of course, that is always a
trade-off between simply doing a change that is correct and works (your
original version) and something like ``formally correctly doing a
change''.

I hope you don't mind if I give comments like these.  You don't have to
subscribe to all of them right now, but perhaps you can keep such things
in the back of your head.


> diff --git a/main.c b/main.c
> index e407926..43e4846 100644
> --- a/main.c
> +++ b/main.c

I forgot in my earlier email: copyright years updates.  Samuel also
forgets them all the time, but perhaps you can do better?  ;-)


> diff --git a/options.c b/options.c
> index beed9f4..c12974c 100644
> --- a/options.c
> +++ b/options.c
> @@ -6,12 +6,12 @@
>     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.
> -
> +

Please try to avoid such whitespace changes.  If there is really
something that needs to be changed, then do that separately.

> @@ -78,7 +80,7 @@ argp_parse_common_options (int key, char *arg,
> struct argp_state *state)
>    static int ulfs_flags = 0, ulfs_mode = 0, ulfs_modified = 0,
>      ulfs_match = 0, ulfs_priority = 0;
>    static struct patternlist ulfs_patternlist =
> -    {
> +    {

Again, and some more below.


>  const char *argp_program_version = STANDARD_HURD_VERSION (unionfs);
> -const char *argp_program_bug_address =
> +const char *argp_program_bug_address =
>  "Gianluca Guida <glguida@gmail.com>";

Please, while you're at it, push a commit to master to change that to
<bug-hurd@gnu.org>.


> diff --git a/unionmount.c b/unionmount.c
> new file mode 100644
> index 0000000..a8939f8
> --- /dev/null
> +++ b/unionmount.c
> @@ -0,0 +1,38 @@
> +/*---------------------------------------------------------------------------*/
> +/* Hurd unionmount */
> +/* The core of unionmount functionality. */
> +/*---------------------------------------------------------------------------*/
> +/* Copyright (C) 2009 Free Software Foundation, Inc.  Written by
> +   Sergiu Ivanov <unlimitedscolobb@gmail.com>.

Please add an empty line between the copyright line and the written by
line.


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

There is an embedded asterisk.  (Switching to GPLv3+ is another
discussion, for later on.)

Same for the other new file, unionmount.h.


Regards,
 Thomas

Attachment: signature.asc
Description: Digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]