git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index
  2018-01-14 10:18 ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
@ 2018-01-14 10:18   ` Nguyễn Thái Ngọc Duy
  0 siblings, 0 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-14 10:18 UTC (permalink / raw)
  To: git
  Cc: t.gummerer, larsxschneider, bmwill, peff, Junio C Hamano,
	szeder.dev, Nguyễn Thái Ngọc Duy

For one thing, we have more consistent cleanup procedure now and always
keep errno intact.

The real purpose is the ability to break out of write_locked_index()
early when mks_tempfile() fails in the next patch. It's more awkward to
do it if this mks_tempfile() is still inside write_shared_index().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 536086e1fe..c568643f55 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2472,31 +2472,18 @@ static int clean_shared_index_files(const char *current_hex)
 }
 
 static int write_shared_index(struct index_state *istate,
-			      struct lock_file *lock, unsigned flags)
+			      struct tempfile **temp)
 {
-	struct tempfile *real_temp;
-	struct tempfile **temp = &real_temp;
 	struct split_index *si = istate->split_index;
 	int ret;
 
-	real_temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
-	if (!real_temp) {
-		hashclr(si->base_sha1);
-		return do_write_locked_index(istate, lock, flags);
-	}
-	temp = &real_temp;
 	move_cache_to_base_index(istate);
 	ret = do_write_index(si->base, *temp, 1);
-	if (ret) {
-		delete_tempfile(temp);
+	if (ret)
 		return ret;
-	}
 	ret = adjust_shared_perm(get_tempfile_path(*temp));
 	if (ret) {
-		int save_errno = errno;
 		error("cannot fix permission bits on %s", get_tempfile_path(*temp));
-		delete_tempfile(temp);
-		errno = save_errno;
 		return ret;
 	}
 	ret = rename_tempfile(temp,
@@ -2567,7 +2554,21 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	new_shared_index = istate->cache_changed & SPLIT_INDEX_ORDERED;
 
 	if (new_shared_index) {
-		ret = write_shared_index(istate, lock, flags);
+		struct tempfile *temp;
+		int saved_errno;
+
+		temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+		if (!temp) {
+			hashclr(si->base_sha1);
+			ret = do_write_locked_index(istate, lock, flags);
+		} else
+			ret = write_shared_index(istate, &temp);
+
+		saved_errno = errno;
+		if (is_tempfile_active(temp))
+			delete_tempfile(&temp);
+		errno = saved_errno;
+
 		if (ret)
 			goto out;
 	}
-- 
2.15.1.600.g899a5f85c6


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 0/3] nd/shared-index-fix update
@ 2018-01-22 11:03 Nguyễn Thái Ngọc Duy
  2018-01-22 11:03 ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-22 11:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, SZEDER Gábor, Jeff King,
	Nguyễn Thái Ngọc Duy

This only changes the last patch to correct the test prerequisite and
a couple minor refinements. Interdiff:

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 5494389dbb..d2a8e0312a 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -401,7 +401,7 @@ done <<\EOF
 0642 -rw-r---w-
 EOF
 
-test_expect_success POSIXPERM 'graceful handling splitting index is not allowed' '
+test_expect_success SANITY 'graceful handling when splitting index is not allowed' '
 	test_create_repo ro &&
 	(
 		cd ro &&
@@ -409,11 +409,11 @@ test_expect_success POSIXPERM 'graceful handling splitting index is not allowed'
 		git update-index --split-index &&
 		test -f .git/sharedindex.*
 	) &&
-	test_when_finished "chmod -R u+w ro" &&
-	chmod -R u-w ro &&
 	cp ro/.git/index new-index &&
+	test_when_finished "chmod u+w ro/.git" &&
+	chmod u-w ro/.git &&
 	GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
-	chmod -R u+w ro &&
+	chmod u+w ro/.git &&
 	rm ro/.git/sharedindex.* &&
 	GIT_INDEX_FILE=new-index git ls-files >actual &&
 	echo initial.t >expected &&

Nguyễn Thái Ngọc Duy (3):
  read-cache.c: change type of "temp" in write_shared_index()
  read-cache.c: move tempfile creation/cleanup out of write_shared_index
  read-cache: don't write index twice if we can't write shared index

 read-cache.c           | 40 ++++++++++++++++++++++------------------
 t/t1700-split-index.sh | 19 +++++++++++++++++++
 2 files changed, 41 insertions(+), 18 deletions(-)

-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index()
  2018-01-22 11:03 [PATCH 0/3] nd/shared-index-fix update Nguyễn Thái Ngọc Duy
@ 2018-01-22 11:03 ` Nguyễn Thái Ngọc Duy
  2018-01-22 11:03 ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy
  2018-01-22 11:03 ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
  2 siblings, 0 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-22 11:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, SZEDER Gábor, Jeff King,
	Nguyễn Thái Ngọc Duy

This local variable 'temp' will be passed in from the caller in the next
patch. To reduce patch noise, let's change its type now while it's still
a local variable and get all the trival conversion out of the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..536086e1fe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2474,30 +2474,32 @@ static int clean_shared_index_files(const char *current_hex)
 static int write_shared_index(struct index_state *istate,
 			      struct lock_file *lock, unsigned flags)
 {
-	struct tempfile *temp;
+	struct tempfile *real_temp;
+	struct tempfile **temp = &real_temp;
 	struct split_index *si = istate->split_index;
 	int ret;
 
-	temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
-	if (!temp) {
+	real_temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+	if (!real_temp) {
 		hashclr(si->base_sha1);
 		return do_write_locked_index(istate, lock, flags);
 	}
+	temp = &real_temp;
 	move_cache_to_base_index(istate);
-	ret = do_write_index(si->base, temp, 1);
+	ret = do_write_index(si->base, *temp, 1);
 	if (ret) {
-		delete_tempfile(&temp);
+		delete_tempfile(temp);
 		return ret;
 	}
-	ret = adjust_shared_perm(get_tempfile_path(temp));
+	ret = adjust_shared_perm(get_tempfile_path(*temp));
 	if (ret) {
 		int save_errno = errno;
-		error("cannot fix permission bits on %s", get_tempfile_path(temp));
-		delete_tempfile(&temp);
+		error("cannot fix permission bits on %s", get_tempfile_path(*temp));
+		delete_tempfile(temp);
 		errno = save_errno;
 		return ret;
 	}
-	ret = rename_tempfile(&temp,
+	ret = rename_tempfile(temp,
 			      git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
 	if (!ret) {
 		hashcpy(si->base_sha1, si->base->sha1);
-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index
  2018-01-22 11:03 [PATCH 0/3] nd/shared-index-fix update Nguyễn Thái Ngọc Duy
  2018-01-22 11:03 ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
@ 2018-01-22 11:03 ` Nguyễn Thái Ngọc Duy
  2018-01-22 11:03 ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
  2 siblings, 0 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-22 11:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, SZEDER Gábor, Jeff King,
	Nguyễn Thái Ngọc Duy

For one thing, we have more consistent cleanup procedure now and always
keep errno intact.

The real purpose is the ability to break out of write_locked_index()
early when mks_tempfile() fails in the next patch. It's more awkward to
do it if this mks_tempfile() is still inside write_shared_index().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 536086e1fe..c568643f55 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2472,31 +2472,18 @@ static int clean_shared_index_files(const char *current_hex)
 }
 
 static int write_shared_index(struct index_state *istate,
-			      struct lock_file *lock, unsigned flags)
+			      struct tempfile **temp)
 {
-	struct tempfile *real_temp;
-	struct tempfile **temp = &real_temp;
 	struct split_index *si = istate->split_index;
 	int ret;
 
-	real_temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
-	if (!real_temp) {
-		hashclr(si->base_sha1);
-		return do_write_locked_index(istate, lock, flags);
-	}
-	temp = &real_temp;
 	move_cache_to_base_index(istate);
 	ret = do_write_index(si->base, *temp, 1);
-	if (ret) {
-		delete_tempfile(temp);
+	if (ret)
 		return ret;
-	}
 	ret = adjust_shared_perm(get_tempfile_path(*temp));
 	if (ret) {
-		int save_errno = errno;
 		error("cannot fix permission bits on %s", get_tempfile_path(*temp));
-		delete_tempfile(temp);
-		errno = save_errno;
 		return ret;
 	}
 	ret = rename_tempfile(temp,
@@ -2567,7 +2554,21 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	new_shared_index = istate->cache_changed & SPLIT_INDEX_ORDERED;
 
 	if (new_shared_index) {
-		ret = write_shared_index(istate, lock, flags);
+		struct tempfile *temp;
+		int saved_errno;
+
+		temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+		if (!temp) {
+			hashclr(si->base_sha1);
+			ret = do_write_locked_index(istate, lock, flags);
+		} else
+			ret = write_shared_index(istate, &temp);
+
+		saved_errno = errno;
+		if (is_tempfile_active(temp))
+			delete_tempfile(&temp);
+		errno = saved_errno;
+
 		if (ret)
 			goto out;
 	}
-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] read-cache: don't write index twice if we can't write shared index
  2018-01-22 11:03 [PATCH 0/3] nd/shared-index-fix update Nguyễn Thái Ngọc Duy
  2018-01-22 11:03 ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
  2018-01-22 11:03 ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy
@ 2018-01-22 11:03 ` Nguyễn Thái Ngọc Duy
  2018-01-22 23:09   ` Junio C Hamano
  2 siblings, 1 reply; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-22 11:03 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, SZEDER Gábor, Jeff King,
	Nguyễn Thái Ngọc Duy

In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is
read only", 2014-06-13), we tried to make sure we can still write an
index, even if the shared index can not be written.

We did so by just calling 'do_write_locked_index()' just before
'write_shared_index()'.  'do_write_locked_index()' always at least
closes the tempfile nowadays, and used to close or commit the lockfile
if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was
introduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of
'write_locked_index()'.

After calling 'write_shared_index()', we call 'write_split_index()',
which calls 'do_write_locked_index()' again, which then tries to use the
closed lockfile again, but in fact fails to do so as it's already
closed. This eventually leads to a segfault.

Make sure to write the main index only once.

[nd: most of the commit message and investigation done by Thomas, I only
tweaked the solution a bit]

Helped-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 read-cache.c           |  5 +++--
 t/t1700-split-index.sh | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index c568643f55..c58c0a978a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2561,8 +2561,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 		if (!temp) {
 			hashclr(si->base_sha1);
 			ret = do_write_locked_index(istate, lock, flags);
-		} else
-			ret = write_shared_index(istate, &temp);
+			goto out;
+		}
+		ret = write_shared_index(istate, &temp);
 
 		saved_errno = errno;
 		if (is_tempfile_active(temp))
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index af9b847761..d2a8e0312a 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -401,4 +401,23 @@ done <<\EOF
 0642 -rw-r---w-
 EOF
 
+test_expect_success SANITY 'graceful handling when splitting index is not allowed' '
+	test_create_repo ro &&
+	(
+		cd ro &&
+		test_commit initial &&
+		git update-index --split-index &&
+		test -f .git/sharedindex.*
+	) &&
+	cp ro/.git/index new-index &&
+	test_when_finished "chmod u+w ro/.git" &&
+	chmod u-w ro/.git &&
+	GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
+	chmod u+w ro/.git &&
+	rm ro/.git/sharedindex.* &&
+	GIT_INDEX_FILE=new-index git ls-files >actual &&
+	echo initial.t >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index
  2018-01-22 11:03 ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
@ 2018-01-22 23:09   ` Junio C Hamano
  2018-01-23  4:06     ` Duy Nguyen
  2018-01-24  9:38     ` [PATCH 0/3] nd/shared-index-fix updates Nguyễn Thái Ngọc Duy
  0 siblings, 2 replies; 12+ messages in thread
From: Junio C Hamano @ 2018-01-22 23:09 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, SZEDER Gábor, Jeff King

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index af9b847761..d2a8e0312a 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -401,4 +401,23 @@ done <<\EOF
>  0642 -rw-r---w-
>  EOF
>  
> +test_expect_success SANITY 'graceful handling when splitting index is not allowed' '

Is SANITY the only prereq we want, or do we want both it and POSIXPERM?

In "git grep SANITY t/" output, we see that they are almost always
used together.

> +	test_create_repo ro &&
> +	(
> +		cd ro &&
> +		test_commit initial &&
> +		git update-index --split-index &&
> +		test -f .git/sharedindex.*
> +	) &&
> +	cp ro/.git/index new-index &&
> +	test_when_finished "chmod u+w ro/.git" &&
> +	chmod u-w ro/.git &&
> +	GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
> +	chmod u+w ro/.git &&
> +	rm ro/.git/sharedindex.* &&
> +	GIT_INDEX_FILE=new-index git ls-files >actual &&
> +	echo initial.t >expected &&
> +	test_cmp expected actual
> +'
> +
>  test_done

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index
  2018-01-22 23:09   ` Junio C Hamano
@ 2018-01-23  4:06     ` Duy Nguyen
  2018-01-24  9:38     ` [PATCH 0/3] nd/shared-index-fix updates Nguyễn Thái Ngọc Duy
  1 sibling, 0 replies; 12+ messages in thread
From: Duy Nguyen @ 2018-01-23  4:06 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, SZEDER Gábor, Jeff King

On Tue, Jan 23, 2018 at 6:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
>> index af9b847761..d2a8e0312a 100755
>> --- a/t/t1700-split-index.sh
>> +++ b/t/t1700-split-index.sh
>> @@ -401,4 +401,23 @@ done <<\EOF
>>  0642 -rw-r---w-
>>  EOF
>>
>> +test_expect_success SANITY 'graceful handling when splitting index is not allowed' '
>
> Is SANITY the only prereq we want, or do we want both it and POSIXPERM?
>
> In "git grep SANITY t/" output, we see that they are almost always
> used together.

SANITY test does more or less the same as this one (chmod then verify)
which is the reason I removed POSIXPERM. Looking at other tests
though, they don't do anything different than what I do here and still
require both SANITY and POSIXPERM. I'm adding POSIXPERM back.

>
>> +     test_create_repo ro &&
>> +     (
>> +             cd ro &&
>> +             test_commit initial &&
>> +             git update-index --split-index &&
>> +             test -f .git/sharedindex.*
>> +     ) &&
>> +     cp ro/.git/index new-index &&
>> +     test_when_finished "chmod u+w ro/.git" &&
>> +     chmod u-w ro/.git &&
>> +     GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
>> +     chmod u+w ro/.git &&
>> +     rm ro/.git/sharedindex.* &&
>> +     GIT_INDEX_FILE=new-index git ls-files >actual &&
>> +     echo initial.t >expected &&
>> +     test_cmp expected actual
>> +'
>> +
>>  test_done



-- 
Duy

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 0/3] nd/shared-index-fix updates
  2018-01-22 23:09   ` Junio C Hamano
  2018-01-23  4:06     ` Duy Nguyen
@ 2018-01-24  9:38     ` Nguyễn Thái Ngọc Duy
  2018-01-24  9:38       ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
                         ` (3 more replies)
  1 sibling, 4 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-24  9:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy

This makes the new test need both prerequisites SANITY and POSIXPERM
instead of just SANITY (on 'pu').  This is how most other tests do it
so we do the same to be safe.

Nguyễn Thái Ngọc Duy (3):
  read-cache.c: change type of "temp" in write_shared_index()
  read-cache.c: move tempfile creation/cleanup out of write_shared_index
  read-cache: don't write index twice if we can't write shared index

 read-cache.c           | 40 ++++++++++++++++++++++------------------
 t/t1700-split-index.sh | 19 +++++++++++++++++++
 2 files changed, 41 insertions(+), 18 deletions(-)

-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index()
  2018-01-24  9:38     ` [PATCH 0/3] nd/shared-index-fix updates Nguyễn Thái Ngọc Duy
@ 2018-01-24  9:38       ` Nguyễn Thái Ngọc Duy
  2018-01-24  9:38       ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-24  9:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy

This local variable 'temp' will be passed in from the caller in the next
patch. To reduce patch noise, let's change its type now while it's still
a local variable and get all the trival conversion out of the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..536086e1fe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2474,30 +2474,32 @@ static int clean_shared_index_files(const char *current_hex)
 static int write_shared_index(struct index_state *istate,
 			      struct lock_file *lock, unsigned flags)
 {
-	struct tempfile *temp;
+	struct tempfile *real_temp;
+	struct tempfile **temp = &real_temp;
 	struct split_index *si = istate->split_index;
 	int ret;
 
-	temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
-	if (!temp) {
+	real_temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+	if (!real_temp) {
 		hashclr(si->base_sha1);
 		return do_write_locked_index(istate, lock, flags);
 	}
+	temp = &real_temp;
 	move_cache_to_base_index(istate);
-	ret = do_write_index(si->base, temp, 1);
+	ret = do_write_index(si->base, *temp, 1);
 	if (ret) {
-		delete_tempfile(&temp);
+		delete_tempfile(temp);
 		return ret;
 	}
-	ret = adjust_shared_perm(get_tempfile_path(temp));
+	ret = adjust_shared_perm(get_tempfile_path(*temp));
 	if (ret) {
 		int save_errno = errno;
-		error("cannot fix permission bits on %s", get_tempfile_path(temp));
-		delete_tempfile(&temp);
+		error("cannot fix permission bits on %s", get_tempfile_path(*temp));
+		delete_tempfile(temp);
 		errno = save_errno;
 		return ret;
 	}
-	ret = rename_tempfile(&temp,
+	ret = rename_tempfile(temp,
 			      git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
 	if (!ret) {
 		hashcpy(si->base_sha1, si->base->sha1);
-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index
  2018-01-24  9:38     ` [PATCH 0/3] nd/shared-index-fix updates Nguyễn Thái Ngọc Duy
  2018-01-24  9:38       ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
@ 2018-01-24  9:38       ` Nguyễn Thái Ngọc Duy
  2018-01-24  9:38       ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
  2018-01-24 18:09       ` [PATCH 0/3] nd/shared-index-fix updates Junio C Hamano
  3 siblings, 0 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-24  9:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy

For one thing, we have more consistent cleanup procedure now and always
keep errno intact.

The real purpose is the ability to break out of write_locked_index()
early when mks_tempfile() fails in the next patch. It's more awkward to
do it if this mks_tempfile() is still inside write_shared_index().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 536086e1fe..c568643f55 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2472,31 +2472,18 @@ static int clean_shared_index_files(const char *current_hex)
 }
 
 static int write_shared_index(struct index_state *istate,
-			      struct lock_file *lock, unsigned flags)
+			      struct tempfile **temp)
 {
-	struct tempfile *real_temp;
-	struct tempfile **temp = &real_temp;
 	struct split_index *si = istate->split_index;
 	int ret;
 
-	real_temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
-	if (!real_temp) {
-		hashclr(si->base_sha1);
-		return do_write_locked_index(istate, lock, flags);
-	}
-	temp = &real_temp;
 	move_cache_to_base_index(istate);
 	ret = do_write_index(si->base, *temp, 1);
-	if (ret) {
-		delete_tempfile(temp);
+	if (ret)
 		return ret;
-	}
 	ret = adjust_shared_perm(get_tempfile_path(*temp));
 	if (ret) {
-		int save_errno = errno;
 		error("cannot fix permission bits on %s", get_tempfile_path(*temp));
-		delete_tempfile(temp);
-		errno = save_errno;
 		return ret;
 	}
 	ret = rename_tempfile(temp,
@@ -2567,7 +2554,21 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	new_shared_index = istate->cache_changed & SPLIT_INDEX_ORDERED;
 
 	if (new_shared_index) {
-		ret = write_shared_index(istate, lock, flags);
+		struct tempfile *temp;
+		int saved_errno;
+
+		temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
+		if (!temp) {
+			hashclr(si->base_sha1);
+			ret = do_write_locked_index(istate, lock, flags);
+		} else
+			ret = write_shared_index(istate, &temp);
+
+		saved_errno = errno;
+		if (is_tempfile_active(temp))
+			delete_tempfile(&temp);
+		errno = saved_errno;
+
 		if (ret)
 			goto out;
 	}
-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3] read-cache: don't write index twice if we can't write shared index
  2018-01-24  9:38     ` [PATCH 0/3] nd/shared-index-fix updates Nguyễn Thái Ngọc Duy
  2018-01-24  9:38       ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
  2018-01-24  9:38       ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy
@ 2018-01-24  9:38       ` Nguyễn Thái Ngọc Duy
  2018-01-24 18:09       ` [PATCH 0/3] nd/shared-index-fix updates Junio C Hamano
  3 siblings, 0 replies; 12+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2018-01-24  9:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy

In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is
read only", 2014-06-13), we tried to make sure we can still write an
index, even if the shared index can not be written.

We did so by just calling 'do_write_locked_index()' just before
'write_shared_index()'.  'do_write_locked_index()' always at least
closes the tempfile nowadays, and used to close or commit the lockfile
if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was
introduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of
'write_locked_index()'.

After calling 'write_shared_index()', we call 'write_split_index()',
which calls 'do_write_locked_index()' again, which then tries to use the
closed lockfile again, but in fact fails to do so as it's already
closed. This eventually leads to a segfault.

Make sure to write the main index only once.

[nd: most of the commit message and investigation done by Thomas, I only
tweaked the solution a bit]

Helped-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 read-cache.c           |  5 +++--
 t/t1700-split-index.sh | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index c568643f55..c58c0a978a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2561,8 +2561,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 		if (!temp) {
 			hashclr(si->base_sha1);
 			ret = do_write_locked_index(istate, lock, flags);
-		} else
-			ret = write_shared_index(istate, &temp);
+			goto out;
+		}
+		ret = write_shared_index(istate, &temp);
 
 		saved_errno = errno;
 		if (is_tempfile_active(temp))
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index af9b847761..cbcefa6e5f 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -401,4 +401,23 @@ done <<\EOF
 0642 -rw-r---w-
 EOF
 
+test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is not allowed' '
+	test_create_repo ro &&
+	(
+		cd ro &&
+		test_commit initial &&
+		git update-index --split-index &&
+		test -f .git/sharedindex.*
+	) &&
+	cp ro/.git/index new-index &&
+	test_when_finished "chmod u+w ro/.git" &&
+	chmod u-w ro/.git &&
+	GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
+	chmod u+w ro/.git &&
+	rm ro/.git/sharedindex.* &&
+	GIT_INDEX_FILE=new-index git ls-files >actual &&
+	echo initial.t >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.16.0.47.g3d9b0fac3a


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/3] nd/shared-index-fix updates
  2018-01-24  9:38     ` [PATCH 0/3] nd/shared-index-fix updates Nguyễn Thái Ngọc Duy
                         ` (2 preceding siblings ...)
  2018-01-24  9:38       ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
@ 2018-01-24 18:09       ` Junio C Hamano
  3 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2018-01-24 18:09 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This makes the new test need both prerequisites SANITY and POSIXPERM
> instead of just SANITY (on 'pu').  This is how most other tests do it
> so we do the same to be safe.
>
> Nguyễn Thái Ngọc Duy (3):
>   read-cache.c: change type of "temp" in write_shared_index()
>   read-cache.c: move tempfile creation/cleanup out of write_shared_index
>   read-cache: don't write index twice if we can't write shared index
>
>  read-cache.c           | 40 ++++++++++++++++++++++------------------
>  t/t1700-split-index.sh | 19 +++++++++++++++++++
>  2 files changed, 41 insertions(+), 18 deletions(-)

Thanks; let's move this to 'next'.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-01-24 18:09 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-22 11:03 [PATCH 0/3] nd/shared-index-fix update Nguyễn Thái Ngọc Duy
2018-01-22 11:03 ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
2018-01-22 11:03 ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy
2018-01-22 11:03 ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
2018-01-22 23:09   ` Junio C Hamano
2018-01-23  4:06     ` Duy Nguyen
2018-01-24  9:38     ` [PATCH 0/3] nd/shared-index-fix updates Nguyễn Thái Ngọc Duy
2018-01-24  9:38       ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
2018-01-24  9:38       ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy
2018-01-24  9:38       ` [PATCH 3/3] read-cache: don't write index twice if we can't write shared index Nguyễn Thái Ngọc Duy
2018-01-24 18:09       ` [PATCH 0/3] nd/shared-index-fix updates Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2018-01-14  9:36 [PATCH v3 4/3] read-cache: don't try to write index if we can't write shared index Duy Nguyen
2018-01-14 10:18 ` [PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index() Nguyễn Thái Ngọc Duy
2018-01-14 10:18   ` [PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index Nguyễn Thái Ngọc Duy

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).