Re: [BUG?] How to make a shared/restricted repo?

[Johan Herland <johan@herland.net>]

On Thursday 26 March 2009, Junio C Hamano wrote:
> To fix the loose object codepath, the earlier patch added a call to
> adjust_shared_perm() to write_loose_object() function, but after looking
> at your 7th patch, I realized that the pattern of file creation inside
> $GIT_DIR typically is to first create a temporary file, write to it, and
> then finish it off by calling move_temp_to_file().  The true purpose of
> the function is to "finalize the file being created", and it is misnamed
> in that it describes how its implementation does it currently (i.e. by
> renaming the temporary file to its final name), but it makes perfect
> sense to call adjust_shared_perm() inside it as a part of finalization. 
> I think this should cover the codepaths your 7th patch fixed without
> actually touching them.

Yes, with one exception:

For the two cases index-pack.c, the chmod(foo, 0444) happens AFTER the
corresponding call to move_temp_to_file(xyzzy, foo). The chmod() in
adjust_shared_perms() would thus be overridden by the chmod(foo, 0444),
which is not what we want. In both cases, I think the chmod(foo, 0444)
can safely be moved up above the call to move_temp_to_file(). Something
like this (although I'm not sure about the semantics of 'from_stdin'):

diff --git a/index-pack.c b/index-pack.c
index 7546822..d289b6a 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -815,6 +815,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		}
 	}
 
+	if (from_stdin)
+		chmod(final_pack_name, 0444);
 	if (final_pack_name != curr_pack_name) {
 		if (!final_pack_name) {
 			snprintf(name, sizeof(name), "%s/pack/pack-%s.pack",
@@ -824,9 +826,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		if (move_temp_to_file(curr_pack_name, final_pack_name))
 			die("cannot store pack file");
 	}
-	if (from_stdin)
-		chmod(final_pack_name, 0444);
 
+	chmod(final_index_name, 0444);
 	if (final_index_name != curr_index_name) {
 		if (!final_index_name) {
 			snprintf(name, sizeof(name), "%s/pack/pack-%s.idx",
@@ -836,7 +837,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		if (move_temp_to_file(curr_index_name, final_index_name))
 			die("cannot store index file");
 	}
-	chmod(final_index_name, 0444);
 
 	if (!from_stdin) {
 		printf("%s\n", sha1_to_hex(sha1));


> Could you eyeball and re-test it?

> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Tested-by: Johan Herland <johan@herland.net>

> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -195,6 +195,8 @@ static int create_default_files(const char
> *template_path)
>
>  	git_config(git_default_config, NULL);
>  	is_bare_repository_cfg = init_is_bare_repository;
> +
> +	/* reading existing config may have overwrote it */

s/overwrote/overwritten/

Otherwise OK, AFAICS.


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net