Re: [PATCH] Make core.sharedRepository more generic (version 2)

[Junio C Hamano <gitster@pobox.com>]

Heikki Orsila <heikki.orsila@iki.fi> writes:

> diff --git a/builtin-init-db.c b/builtin-init-db.c
> index 2854868..8c63295 100644
> --- a/builtin-init-db.c
> +++ b/builtin-init-db.c
> @@ -400,9 +400,9 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  		char buf[10];
>  		/* We do not spell "group" and such, so that
>  		 * the configuration can be read by older version
> -		 * of git.
> +		 * of git. Note, we use octal numbers.
>  		 */
> -		sprintf(buf, "%d", shared_repository);
> +		sprintf(buf, "0%o", shared_repository);

Unconditionally doing this makes the resulting repository unusable by git
1.5.5 and older, even when the user wanted to use the bog standard "git
init --shared".  You can limit the extent of damage if you continue
writing PERM_GROUP and PERM_EVERYBODY out as 1 and 2, and use the new
octal notation only when the user used the settings allowed only with new
git.

> @@ -438,11 +440,46 @@ int git_config_perm(const char *var, const char *value)
>  		    !strcmp(value, "world") ||
>  		    !strcmp(value, "everybody"))
>  			return PERM_EVERYBODY;
> +
> +		/* Parse octal numbers */
> +		i = strtol(value, &endptr, 8);
> +		if (*endptr != 0) {
> +			/* Not an octal number. Maybe true/false? */
> +			if (git_config_bool(var, value))
> +				return PERM_GROUP;
> +			else
> +				return PERM_UMASK;
> +		}
> +
> +		/* Handle compatibility cases */
> +		switch (i) {
> +		case PERM_UMASK:               /* 0 */
> +			return PERM_UMASK;
> +		case OLD_PERM_GROUP:           /* 1 */
> +			return PERM_GROUP;
> +		case OLD_PERM_EVERYBODY:       /* 2 */
> +			return PERM_EVERYBODY;
> +		}

This is valid only because forcing "chmod 0", "chmod 1", nor "chmod 2"
would not make any sense.  We might want to explain that in comment.

> +		/* A filemode value was given: 0xxx */
> +
> +		if ((i & 0600) != 0600)
> +			die("Problem with core.sharedRepository filemode value"
> +			    " (0%.3o).\nThe owner of files must always have "
> +			    "read and write permissions.", i);
> +
> +		if (i & 0002)
> +			warning("core.sharedRepository filemode (0%.3o) is a "
> +				"security threat.\nMasking off write "
> +				"permission for others\n", i);

I am not sure about this.

If the user explicitly asked for world-writable, I think we should allow
it.  "is a" is too strong a statement to make without knowing how the
access to the repository is arranged.  In a setting where there is nothing
but a restricted access over ssh, and "update-paranoid" hook in place, it
may not be a threat at all, and you are forbidding hosting services to use
such an access control model.

> +		/*
> +                 * Mask filemode value. Others can not get write permission.
> +		 * x flags for directories are handled separately.
> +                 */
Whitespace breakages.

> +		return i & 0664;

>  	}
> -	return git_config_bool(var, value);
> +	return PERM_GROUP;

At this point we know value is NULL so always returning PERM_GROUP
probably makes sense.  But if you did

	if (!value)
	        return PERM_GROUP

upfront, you can lose one level of indentation from the major parts
of this function (this is 70% style and 30% readability comment).