Re: [PATCH v2] read-cache: write all indexes with the same permissions

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

Christian Couder <christian.couder@gmail.com> writes:

> "However, as noted in those commits we'd still create the file as
> 0600, and would just re-chmod it only if core.sharedRepository is set
> to "true" or "all". If core.sharedRepository is unset or set to
> "false", then the file mode will not be changed, so without
> core.splitIndex a system with e.g. the umask set to group writeability
> would work for a group member, but not with core.splitIndex set, as
> group members would not be able to access the shared index file.

That is irrelevant.  The repository needs to be configured properly
if it wanted to be used by the members of the group, period.

> It is unfortunately not short lived when core.sharedrepository is
> unset for example as adjust_shared_perm() starts with:
>
> int adjust_shared_perm(const char *path)
> {
>         int old_mode, new_mode;
>
>         if (!get_shared_repository())
>                 return 0;
>
> but get_shared_repository() will return PERM_UMASK which is 0 when
> git_config_get_value("core.sharedrepository", ...) returns a non zero
> value which happens when "core.sharedrepository" is unset.

Which is to say, you get an unwanted result when your repository is
not configured properly.  It is not a news, and I have no sympathy.

Just configure your repository properly and you'll be fine.

>> > Ideally we'd split up the adjust_shared_perm() function to one that
>> > can give us the mode we want so we could just call open() instead of
>> > open() followed by chmod(), but that's an unrelated cleanup.
>>
>> I would drop this paragraph, as I think this is totally incorrect.
>> Imagine your umask is tighter than the target permission.  You ask
>> such a helper function and get "you want 0660".  Doing open(0660)
>> would not help you an iota---you'd need chmod() or fchmod() to
>> adjust the result anyway, which already is done by
>> adjust-shared-perm.
>
> It seems to me that it is not done when "core.sharedrepository" is unset.

So?  You are assuming that the repository is misconfigured and it is
not set to widen the perm bit in the first place, no?

>> > We already have that minor issue with the "index" file
>> > #leftoverbits.
>>
>> The above "Ideally", which I suspect is totally bogus, would show up
>> whey people look for that keyword in the list archive.  This is one
>> of the reasons why I try to write it after at least one person
>> sanity checks that an idea floated is worth remembering.
>
> It was in Ævar's commit message and I thought it might be better to
> keep it so that people looking for that keyword could find the above
> as well as the previous RFC patch.

So do you agree that open(0660) does not guarantee the result will
be group writable, the above "Ideally" is misguided nonsense, and
giving the #leftoverbits label to it will clutter the search result
and harm readers?  That's good.

Thanks.