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

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

[Amit Bakshi]

Hi,

 I'm trying to use this to create a shared repo (group r/w), but it's
not working as expected. The help for git clone says "Set a
configuration variable in the newly-created repository; this takes
effect immediately after the repository is initialized, but before the
remote history is fetched or any files checked out.", but I'm
definitely not seeing this. Checked out files have umask applied.

 When I use the -c flag to git (ie, git -c core.sharedRepository=group
clone ...) it does work as expected.

Not a big deal just thought I'd report it.


Amit

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

[Jonathan Nieder]

(cc-ing Jeff King, "git clone -c" inventor)
Hi,

Amit Bakshi wrote:

>  I'm trying to use this to create a shared repo (group r/w), but it's
> not working as expected. The help for git clone says "Set a
> configuration variable in the newly-created repository; this takes
> effect immediately after the repository is initialized, but before the
> remote history is fetched or any files checked out.", but I'm
> definitely not seeing this. Checked out files have umask applied.
>
> When I use the -c flag to git (ie, git -c core.sharedRepository=group
> clone ...) it does work as expected.

Thanks for an interesting report.

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?

In any event, the lack of such an option steered you toward '-c'.

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.

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.

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 (option_bare) {
 		if (option_mirror)

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

[Jeff King]

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