Re: git clone -c core.sharedRepository=group not working as expected (git v1.8.4 linux/osx)

[Jeff King <peff@peff.net>]

On Tue, Sep 24, 2013 at 12:06:43PM -0700, Jonathan Nieder wrote:

> Part of the underlying problem is that unlike 'git init', 'git clone'
> does not accept a --shared=(true|false|group|...) option.  Worse, it
> does accept a --shared option, with a completely different meaning.
> No better option names are coming immediately to mind, but perhaps
> someone on the list will have ideas that could let 'git clone' and
> 'git init' use the same --share-repository=group option?

Perhaps calling it something like --permissions would be good (IMHO,
that is more descriptive than "--shared" for "git init"). Then both can
respect it, and "init" maintains "--shared" for backwards compatibility.

Calling it --shared-repository does match the config name, but I do not
find that name especially descriptive. But perhaps it is good enough,
and consistency with the existing name is certainly a plus.

> Another problem is that once the configuration is written, it is past
> the point that some of the sharedrepository setting's effect would
> have occured.  The repository, including worktree, object dirs, and so
> on has already been created without g+w and setgid bits set.

Yes. This is as-documented for "clone -c", which claims to act after the
repository is initialized. That being said, I think it is less confusing
for the user for them to take effect as early as possible, so the user
doesn't have to worry.

I cannot think off-hand of any case where the user would actually want
the delayed effect.

> Worse, when we write the new configuration and then re-read it, we
> skip reading some repository-specific configuration
> (core.{repositoryformatversion,sharedrepository,bare,worktree})
> on the assumption that we should already know what their values would
> be.  That assumption is now wrong.

Yes, I think this is simply a bug in 84054f7 (clone: accept config
options on the command line), which assumed that any effects it had
would be picked up by the git_config() call.

> I wonder if something like the following (just a sketch, completely
> untested) would make sense.
> 
> diff --git i/builtin/clone.c w/builtin/clone.c
> index 7ac677d..145a974 100644
> --- i/builtin/clone.c
> +++ w/builtin/clone.c
> @@ -856,7 +856,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	init_db(option_template, INIT_DB_QUIET);
>  	write_config(&option_config);
>  
> +	if (option_bare)
> +		git_config_set("core.bare", "true");
> +
>  	git_config(git_default_config, NULL);
> +	check_repository_format();

If we call check_repository_format() again, we will pick up the new
config. But I think we would still have to go back and fix the paths
created by init_db.

It may be cleaner to teach init_db to add the initial config (and
optionally add "init -c" for testing, though I do not think anyone
particularly cares in the real world).

-Peff