[PATCH 1/3] read-cache: use shared perms when writing shared index

[PATCH 1/3] read-cache: use shared perms when writing shared index

[Christian Couder]

Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
write_shared_index() has been using mks_tempfile() to create the
temporary file that will become the shared index.

But even before that, it looks like the functions used to create this
file didn't call adjust_shared_perm(), which means that the shared
index file has always been created with 600 permissions regardless
of the shared permission settings.

This means that on repositories created with `git init --shared=all`
and using the split index feature one gets an error like:

fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file open failed: Permission denied

when another user performs any operation that reads the shared index.

Let's fix that by using create_tempfile() instead of mks_tempfile()
to create the shared index file.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index bc156a133e..eb71e93aa4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2414,7 +2414,7 @@ static int write_shared_index(struct index_state *istate,
 	struct split_index *si = istate->split_index;
 	int fd, ret;
 
-	fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
+	fd = create_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
 	if (fd < 0) {
 		hashclr(si->base_sha1);
 		return do_write_locked_index(istate, lock, flags);
-- 
2.13.1.516.g05ec6e13aa

[PATCH 2/3] t1301: move movebits() to test-lib-functions.sh

[Christian Couder]

As the movebits() function can be useful outside t1301,
let's move it into test-lib-functions.sh, and while at
it let's rename it test_movebits().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t1301-shared-repo.sh  | 18 +++++++-----------
 t/test-lib-functions.sh |  5 +++++
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1312004f8c..dfece751b5 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -19,10 +19,6 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	)
 '
 
-modebits () {
-	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
-}
-
 for u in 002 022
 do
 	test_expect_success POSIXPERM "shared=1 does not clear bits preset by umask $u" '
@@ -88,7 +84,7 @@ do
 
 		rm -f .git/info/refs &&
 		git update-server-info &&
-		actual="$(modebits .git/info/refs)" &&
+		actual="$(test_modebits .git/info/refs)" &&
 		verbose test "x$actual" = "x-$y"
 
 	'
@@ -98,7 +94,7 @@ do
 
 		rm -f .git/info/refs &&
 		git update-server-info &&
-		actual="$(modebits .git/info/refs)" &&
+		actual="$(test_modebits .git/info/refs)" &&
 		verbose test "x$actual" = "x-$x"
 
 	'
@@ -111,7 +107,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
 	umask 002 &&
 	git update-server-info &&
 	echo "-rw-rw-r--" >expect &&
-	modebits .git/info/refs >actual &&
+	test_modebits .git/info/refs >actual &&
 	test_cmp expect actual
 '
 
@@ -177,7 +173,7 @@ test_expect_success POSIXPERM 'remote init does not use config from cwd' '
 	umask 0022 &&
 	git init --bare child.git &&
 	echo "-rw-r--r--" >expect &&
-	modebits child.git/config >actual &&
+	test_modebits child.git/config >actual &&
 	test_cmp expect actual
 '
 
@@ -187,7 +183,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' '
 	echo whatever >templates/foo &&
 	git init --template=templates &&
 	echo "-rw-rw-rw-" >expect &&
-	modebits .git/foo >actual &&
+	test_modebits .git/foo >actual &&
 	test_cmp expect actual
 '
 
@@ -198,7 +194,7 @@ test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)'
 	test_path_is_missing child.git/foo &&
 	git init --bare --template=../templates child.git &&
 	echo "-rw-rw-rw-" >expect &&
-	modebits child.git/foo >actual &&
+	test_modebits child.git/foo >actual &&
 	test_cmp expect actual
 '
 
@@ -209,7 +205,7 @@ test_expect_success POSIXPERM 'template can set core.sharedrepository' '
 	cp .git/config templates/config &&
 	git init --bare --template=../templates child.git &&
 	echo "-rw-rw-rw-" >expect &&
-	modebits child.git/HEAD >actual &&
+	test_modebits child.git/HEAD >actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5ee124332a..db622c3555 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -216,6 +216,11 @@ test_chmod () {
 	git update-index --add "--chmod=$@"
 }
 
+# Get the modebits from a file.
+test_modebits () {
+	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+}
+
 # Unset a configuration variable, but don't fail if it doesn't exist.
 test_unconfig () {
 	config_dir=
-- 
2.13.1.516.g05ec6e13aa

Re: [PATCH 2/3] t1301: move movebits() to test-lib-functions.sh

[Junio C Hamano]

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

> As the movebits() function can be useful outside t1301,
> let's move it into test-lib-functions.sh, and while at
> it let's rename it test_movebits().

Good thinking, especially on the renaming.

Re: [PATCH 2/3] t1301: move movebits() to test-lib-functions.sh

[Ramsay Jones]

On 22/06/17 20:52, Junio C Hamano wrote:
> Christian Couder <christian.couder@gmail.com> writes:
> 
>> As the movebits() function can be useful outside t1301,
>> let's move it into test-lib-functions.sh, and while at
>> it let's rename it test_movebits().
> 
> Good thinking, especially on the renaming.

Err, except for the commit message! :-D

Both the commit message subject and the commit message body
refer to _move_bits() rather than _mode_bits() etc.
(So, three instances of s/move/mode/).

ATB,
Ramsay Jones

Re: [PATCH 2/3] t1301: move movebits() to test-lib-functions.sh

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 22/06/17 20:52, Junio C Hamano wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>> 
>>> As the movebits() function can be useful outside t1301,
>>> let's move it into test-lib-functions.sh, and while at
>>> it let's rename it test_movebits().
>> 
>> Good thinking, especially on the renaming.
>
> Err, except for the commit message! :-D
>
> Both the commit message subject and the commit message body
> refer to _move_bits() rather than _mode_bits() etc.
> (So, three instances of s/move/mode/).

Wow, I shouldn't give praising reviews too easily ;-)

Good eyes, thanks.

Re: [PATCH 2/3] t1301: move movebits() to test-lib-functions.sh

[Christian Couder]

On Fri, Jun 23, 2017 at 1:09 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 22/06/17 20:52, Junio C Hamano wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> As the movebits() function can be useful outside t1301,
>>> let's move it into test-lib-functions.sh, and while at
>>> it let's rename it test_movebits().
>>
>> Good thinking, especially on the renaming.
>
> Err, except for the commit message! :-D
>
> Both the commit message subject and the commit message body
> refer to _move_bits() rather than _mode_bits() etc.
> (So, three instances of s/move/mode/).

Yeah, sorry about that. This is fixed in the version I will send
really soon now.

[PATCH 3/3] t1700: make sure split-index respects core.sharedrepository

[Christian Couder]

Add a test to check that both the split-index file and the
shared-index file are created using the right permissions
when core.sharedrepository is set.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t1700-split-index.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index af3ec0da5a..a52b92e82b 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -370,4 +370,16 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
 	test $(ls .git/sharedindex.* | wc -l) -le 2
 '
 
+test_expect_success POSIXPERM 'split index respects core.sharedrepository' '
+	git config core.sharedrepository 0666 &&
+	: >seventeen &&
+	git update-index --add seventeen &&
+	echo "-rw-rw-rw-" >expect &&
+	test_modebits .git/index >actual &&
+	test_cmp expect actual &&
+	newest_shared_index=$(ls -t .git/sharedindex.* | head -1) &&
+	test_modebits "$newest_shared_index" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.13.1.516.g05ec6e13aa

Re: [PATCH 3/3] t1700: make sure split-index respects core.sharedrepository

[Junio C Hamano]

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

> Add a test to check that both the split-index file and the
> shared-index file are created using the right permissions
> when core.sharedrepository is set.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t1700-split-index.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index af3ec0da5a..a52b92e82b 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -370,4 +370,16 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
>  	test $(ls .git/sharedindex.* | wc -l) -le 2
>  '
>  
> +test_expect_success POSIXPERM 'split index respects core.sharedrepository' '
> +	git config core.sharedrepository 0666 &&
> +	: >seventeen &&
> +	git update-index --add seventeen &&
> +	echo "-rw-rw-rw-" >expect &&
> +	test_modebits .git/index >actual &&
> +	test_cmp expect actual &&
> +	newest_shared_index=$(ls -t .git/sharedindex.* | head -1) &&

Hmph.  Don't you want to make sure all of them, not just the latest
one, have the expected mode bits?

> +	test_modebits "$newest_shared_index" >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH 3/3] t1700: make sure split-index respects core.sharedrepository

[Christian Couder]

On Thu, Jun 22, 2017 at 9:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Add a test to check that both the split-index file and the
>> shared-index file are created using the right permissions
>> when core.sharedrepository is set.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  t/t1700-split-index.sh | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
>> index af3ec0da5a..a52b92e82b 100755
>> --- a/t/t1700-split-index.sh
>> +++ b/t/t1700-split-index.sh
>> @@ -370,4 +370,16 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now"
>>       test $(ls .git/sharedindex.* | wc -l) -le 2
>>  '
>>
>> +test_expect_success POSIXPERM 'split index respects core.sharedrepository' '
>> +     git config core.sharedrepository 0666 &&
>> +     : >seventeen &&
>> +     git update-index --add seventeen &&
>> +     echo "-rw-rw-rw-" >expect &&
>> +     test_modebits .git/index >actual &&
>> +     test_cmp expect actual &&
>> +     newest_shared_index=$(ls -t .git/sharedindex.* | head -1) &&
>
> Hmph.  Don't you want to make sure all of them, not just the latest
> one, have the expected mode bits?

We use "git config core.sharedrepository 0666" at the beginning of
this test, so it will only apply to the shared index files that are
created after that.

Do you suggest that we test before setting core.sharedrepository that
the existing shared index files all have the default permissions?

Re: [PATCH 3/3] t1700: make sure split-index respects core.sharedrepository

[Junio C Hamano]

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

> We use "git config core.sharedrepository 0666" at the beginning of
> this test, so it will only apply to the shared index files that are
> created after that.
>
> Do you suggest that we test before setting core.sharedrepository that
> the existing shared index files all have the default permissions?

I think it would be sensible to see at least two values appear.
Otherwise we cannot tell if the right value is coming by accident
(because it was the default) or by design (because the configuration
is correctly read).

Re: [PATCH 3/3] t1700: make sure split-index respects core.sharedrepository

[Christian Couder]

On Thu, Jun 22, 2017 at 10:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> We use "git config core.sharedrepository 0666" at the beginning of
>> this test, so it will only apply to the shared index files that are
>> created after that.
>>
>> Do you suggest that we test before setting core.sharedrepository that
>> the existing shared index files all have the default permissions?
>
> I think it would be sensible to see at least two values appear.
> Otherwise we cannot tell if the right value is coming by accident
> (because it was the default) or by design (because the configuration
> is correctly read).

Ok, I think I will use something like this then:

while read -r mode modebits filename; do
    test_expect_success POSIXPERM "split index respects
core.sharedrepository $mode" '
        git config core.sharedrepository "$mode" &&
        : >"$filename" &&
        git update-index --add "$filename" &&
        echo "$modebits" >expect &&
        test_modebits .git/index >actual &&
        test_cmp expect actual &&
        newest_shared_index=$(ls -t .git/sharedindex.* | head -1) &&
        test_modebits "$newest_shared_index" >actual &&
        test_cmp expect actual
    '
done <<\EOF
0666 -rw-rw-rw- seventeen
0642 -rw-r---w- eightteen
EOF

Re: [PATCH 1/3] read-cache: use shared perms when writing shared index

[Junio C Hamano]

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

> Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
> write_shared_index() has been using mks_tempfile() to create the
> temporary file that will become the shared index.
>
> But even before that, it looks like the functions used to create this
> file didn't call adjust_shared_perm(), which means that the shared
> index file has always been created with 600 permissions regardless
> of the shared permission settings.
>
> This means that on repositories created with `git init --shared=all`
> and using the split index feature one gets an error like:
>
> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file open failed: Permission denied
>
> when another user performs any operation that reads the shared index.

Assuming that a "shared" repository setting should allow uses by
different users, the above analysis makes sense to me.

But the conclusion does not.

> Let's fix that by using create_tempfile() instead of mks_tempfile()
> to create the shared index file.
>
> ...
> -	fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
> +	fd = create_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));

So we used to create a temporary file that made sure its name is
unique but now we create sharedindex_XXXXXX with 6 X's literally 
at the end?

Doesn't mks_tempfile() family include a variant where you can give
custom mode?  Better yet, perhaps you can call adjust_shared_perm()
on the path _after_ seeing that mks_tempfile() succeeds (you can ask
get_tempfile_path() which path to adjust, I presume)?

>  	if (fd < 0) {
>  		hashclr(si->base_sha1);
>  		return do_write_locked_index(istate, lock, flags);

Re: [PATCH 1/3] read-cache: use shared perms when writing shared index

[Christian Couder]

On Thu, Jun 22, 2017 at 9:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:

>> Let's fix that by using create_tempfile() instead of mks_tempfile()
>> to create the shared index file.
>>
>> ...
>> -     fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
>> +     fd = create_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
>
> So we used to create a temporary file that made sure its name is
> unique but now we create sharedindex_XXXXXX with 6 X's literally
> at the end?
>
> Doesn't mks_tempfile() family include a variant where you can give
> custom mode?  Better yet, perhaps you can call adjust_shared_perm()
> on the path _after_ seeing that mks_tempfile() succeeds (you can ask
> get_tempfile_path() which path to adjust, I presume)?

Yeah, adjust_shared_perm() is called after mks_tempfile() succeeds, in
the next version.

Re: [PATCH 1/3] read-cache: use shared perms when writing shared index

[Ævar Arnfjörð Bjarmason]

On Thu, 22 Jun 2017 12:51:54 -0700 Junio wrote:

> > Let's fix that by using create_tempfile() instead of mks_tempfile()
> > to create the shared index file.
> >
> > ...
> > -     fd = mks_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
> > +     fd = create_tempfile(&temporary_sharedindex, git_path("sharedindex_XXXXXX"));
>
> So we used to create a temporary file that made sure its name is
> unique but now we create sharedindex_XXXXXX with 6 X's literally
> at the end?

I'm looking at some of this again. Yeah that was a bug in Christian's
code, but interestingly if you just create literal sharedindex_XXXXXX
files (don't replace the X's) the whole test suite passes under
GIT_TEST_SPLIT_INDEX=true

That seems like a major blindspot, i.e. we don't seem to have tests that
stress test the case of >1 split index.

[RFC/PATCH] read-cache: write all indexes with the same permissions

[Ævar Arnfjörð Bjarmason]

Change the code that writes out the shared index to use
create_tempfile() instead of mks_tempfile();

The create_tempfile() function is used to write out the main
.git/index (via .git/index.lock) using lock_file(). The
create_tempfile() function respects the umask, whereas the
mks_tempfile() function will create files with 0600 permissions.

A bug related to this was spotted, fixed and tested for in
df801f3f9f ("read-cache: use shared perms when writing shared index",
2017-06-25) and 3ee83f48e5 ("t1700: make sure split-index respects
core.sharedrepository", 2017-06-25).

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, but not with
core.splitIndex set.

Let's instead make the two consistent by using create_tempfile(). This
allows us to remove the code added in df801f3f9f (subsequently
modified in 59f9d2dd60 ("read-cache.c: move tempfile creation/cleanup
out of write_shared_index", 2018-01-14)) as redundant. The
create_tempfile() function itself calls adjust_shared_perm().

Now we're not leaking the implementation detail that we're using a
mkstemp()-like API for something that's not really a mkstemp()
use-case. See c18b80a0e8 ("update-index: new options to enable/disable
split index mode", 2014-06-13) for the initial implementation which
used mkstemp() without a wrapper.

One thing I was paranoid about when making this change was not
introducing a race condition where with
e.g. core.sharedRepository=0600 we'd do something different for
"index" v.s. "sharedindex.*", as the former has a *.lock file, not the
latter.

But I'm confident that we're exposing no such edge-case. With a user
umask of e.g. 0022 and core.sharedRepository=0600 we initially create
both "index' and "sharedindex.*" files that are globally readable, but
re-chmod them while they're still empty.

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 <avarab@gmail.com>
---

I won't have time to finish this today, as noted in
https://public-inbox.org/git/874lcl2e9t.fsf@evledraar.gmail.com/
there's a pretty major bug here in that we're now writing out literal
sharedindex_XXXXXX files.

Obviously that needs to be fixed, and the fix is trivial, I can use
another one of the mks_*() functions with the same mode we use to
create the index.

But we really ought to have tests for the bug this patch introduces,
and as noted in the E-Mail linked above we don't.

So hopefully Duy or someone with more knowledge of the split index
will chime in to say what's missing there...

 read-cache.c           |  7 +------
 t/t1700-split-index.sh | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f3a848d61c..7135537554 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3074,11 +3074,6 @@ static int write_shared_index(struct index_state *istate,
 	ret = do_write_index(si->base, *temp, 1);
 	if (ret)
 		return ret;
-	ret = adjust_shared_perm(get_tempfile_path(*temp));
-	if (ret) {
-		error("cannot fix permission bits on %s", get_tempfile_path(*temp));
-		return ret;
-	}
 	ret = rename_tempfile(temp,
 			      git_path("sharedindex.%s", oid_to_hex(&si->base->oid)));
 	if (!ret) {
@@ -3159,7 +3154,7 @@ 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"));
+		temp = create_tempfile(git_path("sharedindex_XXXXXX"));
 		if (!temp) {
 			oidclr(&si->base_oid);
 			ret = do_write_locked_index(istate, lock, flags);
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 2ac47aa0e4..fa1d3d468b 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -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" '
-- 
2.19.1.1182.g4ecb1133ce

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

[Duy Nguyen]

On Tue, Nov 13, 2018 at 4:32 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> I won't have time to finish this today, as noted in
> https://public-inbox.org/git/874lcl2e9t.fsf@evledraar.gmail.com/
> there's a pretty major bug here in that we're now writing out literal
> sharedindex_XXXXXX files.

It's not the end of the world because create_tempfile opens with
O_EXCL so if two processes try to create it at the same time, one will
fail, but no corruption or such.

> Obviously that needs to be fixed, and the fix is trivial, I can use
> another one of the mks_*() functions with the same mode we use to
> create the index.
>
> But we really ought to have tests for the bug this patch introduces,
> and as noted in the E-Mail linked above we don't.
>
> So hopefully Duy or someone with more knowledge of the split index
> will chime in to say what's missing there...

I don't have any bright idea how to catch the literal _XXXXX file.
It's a temporary file and will not last long enough for us to verify
unless we intercept open() calls with LD_PRELOAD.
-- 
Duy

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 13 2018, Duy Nguyen wrote:

> On Tue, Nov 13, 2018 at 4:32 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> I won't have time to finish this today, as noted in
>> https://public-inbox.org/git/874lcl2e9t.fsf@evledraar.gmail.com/
>> there's a pretty major bug here in that we're now writing out literal
>> sharedindex_XXXXXX files.
>
> It's not the end of the world because create_tempfile opens with
> O_EXCL so if two processes try to create it at the same time, one will
> fail, but no corruption or such.

Right, no race there.

>> Obviously that needs to be fixed, and the fix is trivial, I can use
>> another one of the mks_*() functions with the same mode we use to
>> create the index.
>>
>> But we really ought to have tests for the bug this patch introduces,
>> and as noted in the E-Mail linked above we don't.
>>
>> So hopefully Duy or someone with more knowledge of the split index
>> will chime in to say what's missing there...
>
> I don't have any bright idea how to catch the literal _XXXXX file.
> It's a temporary file and will not last long enough for us to verify
> unless we intercept open() calls with LD_PRELOAD.

Sorry for being unclear. I don't mean how can we catch this specific
bug, that would be uninteresting and hard to test for.

I'm asking whether the bug in this patch isn't revealing an existing
issue with us not having any tests for N number of sharedindex.*
files. I.e. we have >1 of them, merge them and prune them, don't we?

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

[Christian Couder]

On Tue, Nov 13, 2018 at 6:34 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Tue, Nov 13 2018, Duy Nguyen wrote:
>
> > On Tue, Nov 13, 2018 at 4:32 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> > I don't have any bright idea how to catch the literal _XXXXX file.
> > It's a temporary file and will not last long enough for us to verify
> > unless we intercept open() calls with LD_PRELOAD.
>
> Sorry for being unclear. I don't mean how can we catch this specific
> bug, that would be uninteresting and hard to test for.
>
> I'm asking whether the bug in this patch isn't revealing an existing
> issue with us not having any tests for N number of sharedindex.*
> files. I.e. we have >1 of them, merge them and prune them, don't we?

I think we shouldn't have many of them. Usually we should have just
one, though it's possible that switching the shared index files
feature on and off several times or using temporary index files could
create more than one.

And there is clean_shared_index_files() which calls
should_delete_shared_index() to make sure they are regularly cleaned
up.

Anyway it's a different topic and according to what we privately
discussed I just sent
https://public-inbox.org/git/20181116173105.21784-1-chriscool@tuxfamily.org/
to fix the current issue.

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

[SZEDER Gábor]

On Fri, Nov 16, 2018 at 06:41:43PM +0100, Christian Couder wrote:
> On Tue, Nov 13, 2018 at 6:34 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:

> > I'm asking whether the bug in this patch isn't revealing an existing
> > issue with us not having any tests for N number of sharedindex.*
> > files. I.e. we have >1 of them, merge them and prune them, don't we?

We don't merge shared index files, but write a new one.

> I think we shouldn't have many of them. Usually we should have just
> one, though it's possible that switching the shared index files
> feature on and off several times or using temporary index files could
> create more than one.
>
> And there is clean_shared_index_files() which calls
> should_delete_shared_index() to make sure they are regularly cleaned
> up.

I would think that it's more common to have more than one shared index
files, because a new shared index is written when the number of
entries in the split index reaches the threshold given in
'splitIndex.maxPercentChange'.  The old ones are kept until they
expire, and 'splitIndex.sharedIndexExpire' defaults to 2 weeks (and
can even be be set to "never").

With the default 20% threshold a new shared index is written rather
frequently with our usual small test-repos:

  $ git init
  $ git update-index --split-index
  $ ls -1 .git/*index*
  .git/index
  .git/sharedindex.4370042739b31cd17a5c5cd6043a77c9a00df113
  $ echo 1 >file
  $ git add file
  $ git commit -q -m 1
  $ echo 2 >file
  $ git commit -q -m 2 file
  $ echo 3 >file
  $ git commit -q -m 3 file
  $ ls -1 .git/*index*
  .git/index
  .git/sharedindex.4370042739b31cd17a5c5cd6043a77c9a00df113
  .git/sharedindex.6aedbf71b1a6bdc0018078ec7571e1b21ba4b954
  .git/sharedindex.b9106e9b82a818a0e4e9148224fc44ea98f488a1
  .git/sharedindex.bad0b75d270a431b9e961cfc15df6ec935a67be8

> Anyway it's a different topic and according to what we privately
> discussed I just sent
> https://public-inbox.org/git/20181116173105.21784-1-chriscool@tuxfamily.org/
> to fix the current issue.

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

[Duy Nguyen]

On Fri, Nov 16, 2018 at 8:07 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Fri, Nov 16, 2018 at 06:41:43PM +0100, Christian Couder wrote:
> > On Tue, Nov 13, 2018 at 6:34 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
>
> > > I'm asking whether the bug in this patch isn't revealing an existing
> > > issue with us not having any tests for N number of sharedindex.*
> > > files. I.e. we have >1 of them, merge them and prune them, don't we?
>
> We don't merge shared index files, but write a new one.

True. They are immutable like git objects.

> With the default 20% threshold a new shared index is written rather
> frequently with our usual small test-repos:

Side note. Split index is definitely not meant for small repos. But
maybe we should have a lower limit (in terms of absolute number of
entries) that prevent splitting. This splitting seems excessive.

>   $ git init
>   $ git update-index --split-index
>   $ ls -1 .git/*index*
>   .git/index
>   .git/sharedindex.4370042739b31cd17a5c5cd6043a77c9a00df113
>   $ echo 1 >file
>   $ git add file
>   $ git commit -q -m 1
>   $ echo 2 >file
>   $ git commit -q -m 2 file
>   $ echo 3 >file
>   $ git commit -q -m 3 file
>   $ ls -1 .git/*index*
>   .git/index
>   .git/sharedindex.4370042739b31cd17a5c5cd6043a77c9a00df113
>   .git/sharedindex.6aedbf71b1a6bdc0018078ec7571e1b21ba4b954
>   .git/sharedindex.b9106e9b82a818a0e4e9148224fc44ea98f488a1
>   .git/sharedindex.bad0b75d270a431b9e961cfc15df6ec935a67be8
-- 
Duy

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

[Christian Couder]

On Fri, Nov 16, 2018 at 8:20 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Fri, Nov 16, 2018 at 8:07 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> > With the default 20% threshold a new shared index is written rather
> > frequently with our usual small test-repos:
>
> Side note. Split index is definitely not meant for small repos.

I very much agree with that. It makes sense to use them only for big
repos and big repos usually don't pass a 20% threshold very often.

> But
> maybe we should have a lower limit (in terms of absolute number of
> entries) that prevent splitting. This splitting seems excessive.

I would agree if split index was the default mode or if our goal was
to eventually make it the default mode.

Or it could be a new "mixed" mode for core.splitIndex (which might
eventually become the default mode) to have no split-index as long as
the repo stays under a lower limit and to automatically use
split-index when the repo gets over the limit.

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

[SZEDER Gábor]

On Sat, Nov 17, 2018 at 07:52:30AM +0100, Christian Couder wrote:
> On Fri, Nov 16, 2018 at 8:20 PM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Fri, Nov 16, 2018 at 8:07 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > > With the default 20% threshold a new shared index is written rather
> > > frequently with our usual small test-repos:
> >
> > Side note. Split index is definitely not meant for small repos.
> 
> I very much agree with that. It makes sense to use them only for big
> repos and big repos usually don't pass a 20% threshold very often.

But our test suite does use very small repositories, so perhaps we
have been already testing what Ævar wanted to test?  (Though I didn't
quite understood what that was; and we likely don't do so explicitly,
but only by chance with GIT_TEST_SPLIT_INDEX=1.)

> > But
> > maybe we should have a lower limit (in terms of absolute number of
> > entries) that prevent splitting. This splitting seems excessive.
> 
> I would agree if split index was the default mode or if our goal was
> to eventually make it the default mode.

Same here.  If you don't need split index, i.e. don't have huge repos,
then don't enable it in the first place.  And if it is enabled in a
small repo, then the extra effort to write a new shared index is
negligible, and the space wasted for those small files doesn't really
matter (though arguably the output from a 'ls .git' would be
surprising...  which, at the same time, would be a good motivating
factor to turn the feature off).