read-cache: write all indexes with the same permissions

read-cache.c +1 -0

+               temp = mks_tempfile_sm(git_path("sharedindex_XXXXXX"), 0, 0666);
                if (!temp) {
                        oidclr(&si->base_oid);
                        ret = do_write_locked_index(istate, lock, flags);

Quoted 8 lines · Christian Couder

read-cache.c +0 -0

index 8c924506dd..ea80600bff 100644
@@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
                struct tempfile *temp;
                int saved_errno;
 

read-cache.c int write_locked_index(struct index_state *istate, struct lock_file *lock, · line 3168

Quoted 2 lines

read-cache.c +1 -1

-               temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+               /* Same permissions as the main .git/index file */

If the permission is already correct from the beginning (of this temp file), should df801f3f9f be reverted since we don't need to adjust permission anymore?

Or does $GIT_DIR/index go through the same adjust_shared_perm() anyway in the end, which means df801f3f9f must stay?

If so, perhaps clarify
that in the commit message.

Quoted 11 lines · Duy Nguyen

Quoted 8 lines · Christian Couder

read-cache.c +0 -0

index 8c924506dd..ea80600bff 100644
@@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
                struct tempfile *temp;
                int saved_errno;
 

Quoted 6 lines

read-cache.c int write_locked_index(struct index_state *istate, struct lock_file *lock, · line 3168

Quoted 2 lines

read-cache.c +1 -1

-               temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+               /* Same permissions as the main .git/index file */

If the permission is already correct from the beginning (of this temp file), should df801f3f9f be reverted since we don't need to adjust permission anymore?

df801f3f9f (read-cache: use shared perms when writing shared index, 2017-06-25) was fixing the bug that permissions of the shared index file did not take into account the shared permissions (which are about core.sharedRepository; "shared" has a different meaning in "shared index file" and in "shared permissions").

This fix only changes permissions before the shared permissions are taken into account (so before adjust_shared_perm() is called).

Quoted 2 lines

Or does $GIT_DIR/index go through the same adjust_shared_perm() anyway in the end, which means df801f3f9f must stay?

Yeah, $GIT_DIR/index goes through adjust_shared_perm() too because create_tempfile() calls adjust_shared_perm(). So indeed df801f3f9f must stay.

Let's instead make the two consistent by using mks_tempfile_sm() and passing 0666 in its mode argument.

t/t1700-split-index.sh +0 -0

index 2ac47aa0e4..fa1d3d468b 100755
@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
 	test $(ls .git/sharedindex.* | wc -l) -le 2
 '

t/t1700-split-index.sh +11 -0

+		case "$shared" in
+		*" "*)
+			# we have more than one???
+			false ;;
+		*)
+			test_modebits "$shared" >split_index_mode &&
+			test_cmp index_mode split_index_mode ;;
+		esac
+	)
+'
+
 while read -r mode modebits
 do
 	test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" '

Signature · 1 line

2.19.1.1053.g063ed687ac

Quoted 7 lines · Christian Couder

t/t1700-split-index.sh +0 -0

index 2ac47aa0e4..fa1d3d468b 100755
@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
 	test $(ls .git/sharedindex.* | wc -l) -le 2
 '

t/t1700-split-index.sh test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now" · line 392

Quoted 10 lines

t/t1700-split-index.sh +9 -0

 
+test_expect_success POSIXPERM 'same mode for index & split index' '
+	git init same-mode &&
+	(
+		cd same-mode &&
+		test_commit A &&
+		test_modebits .git/index >index_mode &&
+		test_must_fail git config core.sharedRepository &&
+		git -c core.splitIndex=true status &&
+		shared=$(ls .git/sharedindex.*) &&

I think the command substitution and 'ls' are unnecessary, and

shared=.git/sharedindex.*

would work as well.

Quoted 17 lines

t/t1700-split-index.sh +11 -1

+		case "$shared" in
+		*" "*)
+			# we have more than one???
+			false ;;
+		*)
+			test_modebits "$shared" >split_index_mode &&
+			test_cmp index_mode split_index_mode ;;
+		esac
+	)
+'
+
 while read -r mode modebits
 do
 	test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" '
--

2.19.1.1053.g063ed687ac

Quoted 7 lines · Christian Couder

t/t1700-split-index.sh +0 -0

index 2ac47aa0e4..fa1d3d468b 100755
@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
      test $(ls .git/sharedindex.* | wc -l) -le 2
 '

t/t1700-split-index.sh test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now" · line 392

Quoted 10 lines

t/t1700-split-index.sh +9 -0

 
+test_expect_success POSIXPERM 'same mode for index & split index' '
+     git init same-mode &&
+     (
+             cd same-mode &&
+             test_commit A &&
+             test_modebits .git/index >index_mode &&
+             test_must_fail git config core.sharedRepository &&
+             git -c core.splitIndex=true status &&
+             shared=$(ls .git/sharedindex.*) &&

I think the command substitution and 'ls' are unnecessary, and

shared=.git/sharedindex.*

would work as well.

Quoted 10 lines

t/t1700-split-index.sh test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now" · line 392

Quoted 9 lines

t/t1700-split-index.sh +9 -0

+test_expect_success POSIXPERM 'same mode for index & split index' '
+     git init same-mode &&
+     (
+             cd same-mode &&
+             test_commit A &&
+             test_modebits .git/index >index_mode &&
+             test_must_fail git config core.sharedRepository &&
+             git -c core.splitIndex=true status &&
+             shared=$(ls .git/sharedindex.*) &&

Quoted 5 lines

I think the command substitution and 'ls' are unnecessary, and

shared=.git/sharedindex.*

would work as well.

If there is no shared index file with the above we would get:

shared=.git/sharedindex.*

$ echo $shared
.git/sharedindex.*

which seems bug prone to me.

Quoted 22 lines · SZEDER Gábor

Quoted 7 lines · Christian Couder

t/t1700-split-index.sh +0 -0

index 2ac47aa0e4..fa1d3d468b 100755
@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
      test $(ls .git/sharedindex.* | wc -l) -le 2
 '

t/t1700-split-index.sh test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now" · line 392

Quoted 10 lines

t/t1700-split-index.sh +9 -0

 
+test_expect_success POSIXPERM 'same mode for index & split index' '
+     git init same-mode &&
+     (
+             cd same-mode &&
+             test_commit A &&
+             test_modebits .git/index >index_mode &&
+             test_must_fail git config core.sharedRepository &&
+             git -c core.splitIndex=true status &&
+             shared=$(ls .git/sharedindex.*) &&

I think the command substitution and 'ls' are unnecessary, and

Quoted 3 lines

shared=.git/sharedindex.*

would work as well.

If there is no shared index file with the above we would get:

shared=.git/sharedindex.*

$ echo $shared
.git/sharedindex.*

which seems bug prone to me.

However, as noted in those commits we'd still create the file as 0600, and would just re-chmod it depending on the setting of core.sharedRepository. So without core.splitIndex a system with e.g. the umask set to group writeability would work for the members of the group, but not with core.splitIndex set, as members of the group would not be able to access the shared index file.

Let's instead make the two consistent by using mks_tempfile_sm() and passing 0666 in its mode argument.

Note that we cannot use the create_tempfile() function itself that is used to write the main ".git/index" file because we want the XXXXXX part of the "sharedindex_XXXXXX" argument to be replaced by a pseudo random value and create_tempfile() doesn't do that.

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.

We already have that minor issue with the "index" file #leftoverbits.

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by Christian Couder <[email protected]>

---

This is a simpler fix iterating from Ævar's RFC patch and the following discussions:

https://public-inbox.org/git/[email protected]/

 read-cache.c           |  3 ++-
 t/t1700-split-index.sh | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

read-cache.c +2 -1

index 8c924506dd..ea80600bff 100644
@@ -3165,7 +3165,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 		struct tempfile *temp;
 		int saved_errno;
 
-		temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+		/* Same permissions as the main .git/index file */
+		temp = mks_tempfile_sm(git_path("sharedindex_XXXXXX"), 0, 0666);
 		if (!temp) {
 			oidclr(&si->base_oid);
 			ret = do_write_locked_index(istate, lock, flags);

t/t1700-split-index.sh +20 -0

index 2ac47aa0e4..fa1d3d468b 100755
@@ -381,6 +381,26 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
 	test $(ls .git/sharedindex.* | wc -l) -le 2
 '
 
+test_expect_success POSIXPERM 'same mode for index & split index' '
+	git init same-mode &&
+	(
+		cd same-mode &&
+		test_commit A &&
+		test_modebits .git/index >index_mode &&
+		test_must_fail git config core.sharedRepository &&
+		git -c core.splitIndex=true status &&
+		shared=$(ls .git/sharedindex.*) &&
+		case "$shared" in
+		*" "*)
+			# we have more than one???
+			false ;;
+		*)
+			test_modebits "$shared" >split_index_mode &&
+			test_cmp index_mode split_index_mode ;;
+		esac
+	)
+'
+
 while read -r mode modebits
 do
 	test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" '

Quoted 6 lines · Christian Couder

However, as noted in those commits we'd still create the file as 0600, and would just re-chmod it depending on the setting of core.sharedRepository. So without core.splitIndex a system with e.g. the umask set to group writeability would work for the members of the group, but not with core.splitIndex set, as members of the group would not be able to access the shared index file.

I am not sure what the above wants to say.

If we are not making
necessary call to adjust-shared-perm,

then it is irrelevant that the
lack of the call does not immediately cause an apparent problem for users who happens to have non-restrictive group perm bit in their umask. Another group member whose umask is tighter will eventually use the repository and end up creating a file unreadable to group members.

Are you saying that we _lack_ necessary call when core.sharedRepository is set?

If so, a commit that fixes such a bug would be the best place to have a paragraph like the above. If not, the above description simply misleads the readers.

Quoted 2 lines

Let's instead make the two consistent by using mks_tempfile_sm() and passing 0666 in its mode argument.

On the other hand, this is a relevant description; this patch kills an inconsistency that is very short lived (I am assuming that there is no bug in the current code before this patch and we make necessary calls to adjust-shared-perm when core.sharedrepository is set).

Quoted 4 lines

Note that we cannot use the create_tempfile() function itself that is used to write the main ".git/index" file because we want the XXXXXX part of the "sharedindex_XXXXXX" argument to be replaced by a pseudo random value and create_tempfile() doesn't do that.

Sure. Pseudo-random-ness is less important than the resulting filename being unique. "Because we are asking for a unique file to be created, we cannot use create_tempfile() interface that is designed to be used to create a file with known name."

But is that really worth saying, I wonder.

Quoted 3 lines

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.

Quoted 2 lines

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.

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

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.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 7 lines · Christian Couder

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

Quoted 10 lines

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;

Quoted 3 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 6 lines

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?

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Do you mean that you don't agree that following should always create both "foo" and e.g. ".git/refs/heads/master" with the same 644 (-rw-rw-r--) mode:

    (
        rm -rf /tmp/repo &&
        umask 022 &&
        git init /tmp/repo &&
        cd /tmp/repo &&
        echo hi >foo &&
        git add foo &&
        git commit -m"first"
    )

To me what we should do with the standard umask and what core.sharedRepository are for are completely different things.

Quoted 14 lines · Ævar Arnfjörð Bjarmason

Do you mean that you don't agree that following should always create both "foo" and e.g. ".git/refs/heads/master" with the same 644 (-rw-rw-r--) mode:

    (
        rm -rf /tmp/repo &&
        umask 022 &&
        git init /tmp/repo &&
        cd /tmp/repo &&
        echo hi >foo &&
        git add foo &&
        git commit -m"first"
    )

Quoted 2 lines

To me what we should do with the standard umask and what core.sharedRepository are for are completely different things.

Ahh, of course. If you put it that way, I do agree that it gives us a valid use case where core.sharedRepository is false and the umask of repository owner is set to 022 (or anything that does not allow write to group or others, and allows read to group) to let group members only peek but not touch the contents of the repository.

I think I was distracted by the mention of ore.sharedRepository in the proposed log message.