Re: [PATCH 6/9] builtin-init-db.c: use parse_options()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 24, 2008, 17:07 UTC
Message-ID: <alpine.DEB.1.00.0807241801030.8986@racer>
In-Reply-To: <4888AAB2.5050007@free.fr>

Hi,

On Thu, 24 Jul 2008, Olivier Marin wrote:

Show 5 quoted lines

> Michele Ballabio a ??rit :
> 
> > +		OPT_BOOLEAN(0, "bare", &bare, "set up a bare repo"),
> 
> s/set up/setup/

No.  "setup" is a noun.

Show 11 quoted lines

> > +		{ OPTION_CALLBACK, 0, "shared", &shared_repository,
> > +		  "type", "type of shared repository",
> 
> What about "permissions", "setup a shared repository"?
> 
> > +		  PARSE_OPT_OPTARG, parse_opt_shared_cb, PERM_GROUP },
> 
> Are you sure the default value is really used here?
> 
> Also, perhaps we can play it safer by avoiding changing "share_repository"
> directly.

I do not see how that would be any safer.

> $ git init -> shared_repository == PERM_UMASK
> $ git init --shared --no-shared -> shared_repository == 0
> 
> It works because PERM_UMASK == 0, but it is a side effect. Don't you think?

Then the callback is wrong, too. I think, however, that it is by design, and correct.

We rely on shared_repository == 0 for non-shared repositories _almost everywhere_.

Show 8 quoted lines

> > +		OPT_BIT('q', "quiet", &flags, "be quiet", INIT_DB_QUIET),
> 
> OPT__QUIET(&quiet),
> 
> if (quiet)
> 	flags |= INIT_DB_QUIET;
> 
> to use the same quiet option everywhere?

Why? Doesn't make it more readable, I think. I'd rather have 3 lines less.

Ciao, Dscho