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

From: Jeff King <peff@peff.net>
Date: Sep 25, 2013, 08:18 UTC
Message-ID: <20130925081844.GA23238@sigill.intra.peff.net>
In-Reply-To: <20130924190643.GL9464@google.com>

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 16 quoted lines

> 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