[PATCH 0/3] nd/shared-index-fix update

[PATCH 0/3] nd/shared-index-fix update

[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

[PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index()

[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

[PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index

[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

[PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[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

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Junio C Hamano]

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

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Duy Nguyen]

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

[PATCH 0/3] nd/shared-index-fix updates

[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

[PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index()

[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

[PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index

[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

[PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[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

Re: [PATCH 0/3] nd/shared-index-fix updates

[Junio C Hamano]

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

Re: [PATCH v3 4/3] read-cache: don't try to write index if we can't write shared index

[Duy Nguyen]

On Sun, Jan 14, 2018 at 5:37 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> 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()' from
> '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.
>
> In the current version, git will in fact segfault if it can't create a
> new file in $gitdir, and this feature seems to never have worked in the
> first place.
>
> Ever since introducing the split index feature, nobody has complained
> about this failing, and it really just papers over repositories that
> will sooner or later need fixing anyway.

Actually there's one valid case for this: you're accessing a read-only
$GIT_DIR (.e.g maybe from a web server cgi script which may be run by
user nobody or something) and creating a temporary index _outside_
$GIT_DIR. I used to do this when I wanted to do "git grep" on some
SHA-1 a couple times. Doing "git grep <SHA-1>" directly (a couple
times) pays full cost for walking trees. If you prepare an index
first, you pay it only once.

> Therefore just make being unable to write the split index a proper
> error, and have users fix their repositories instead of trying (but
> failing) to paper over the error.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  read-cache.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index d13ce83794..a9c8facdfd 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2471,18 +2471,15 @@ static int clean_shared_index_files(const char *current_hex)
>         return 0;
>  }
>
> -static int write_shared_index(struct index_state *istate,
> -                             struct lock_file *lock, unsigned flags)
> +static int write_shared_index(struct index_state *istate)
>  {
>         struct tempfile *temp;
>         struct split_index *si = istate->split_index;
>         int ret;
>
>         temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
> -       if (!temp) {
> -               hashclr(si->base_sha1);
> -               return do_write_locked_index(istate, lock, flags);

I think this code tries to do what's done near the beginning of
write_locked_index() where we also bail out early:

-- 8< --
        if (!si || alternate_index_output ||
            (istate->cache_changed & ~EXTMASK)) {
                if (si)
                        hashclr(si->base_sha1);
                ret = do_write_locked_index(istate, lock, flags);
                goto out;
        }
-- 8< --

the only difference is it does not realize that it can't do "goto
out;" like that code unless something goes wrong. I'll try to prepare
a patch that move tempfile creation out of write_shared_index()
instead. Patches coming in a bit..
-- 
Duy

[PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index()

[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>
---
 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.15.1.600.g899a5f85c6

[PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[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..5494389dbb 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 'graceful handling splitting index is not allowed' '
+	test_create_repo ro &&
+	(
+		cd ro &&
+		test_commit initial &&
+		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 &&
+	GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index &&
+	chmod -R u+w ro &&
+	rm ro/.git/sharedindex.* &&
+	GIT_INDEX_FILE=new-index git ls-files >actual &&
+	echo initial.t >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.15.1.600.g899a5f85c6

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[SZEDER Gábor]

This series, queued as 'nd/shared-index-fix', makes the 32 bit Linux
build job fail on Travis CI.  Unfortunately, all it can tell us about
the failure is this:

  Test Summary Report
  -------------------
  t1700-split-index.sh                             (Wstat: 256 Tests: 23
  Failed: 1)
    Failed test:  23
    Non-zero exit status: 1
  Files=809, Tests=18491, 401 wallclock secs ( 7.22 usr  1.60 sys + 263.16
  cusr 49.58 csys = 321.56 CPU)
  Result: FAIL

because it can't access the test's verbose log due to lack of
permissions.


  https://travis-ci.org/git/git/jobs/329681826#L2074

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Duy Nguyen]

On Thu, Jan 18, 2018 at 6:36 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> This series, queued as 'nd/shared-index-fix', makes the 32 bit Linux
> build job fail on Travis CI.  Unfortunately, all it can tell us about
> the failure is this:
>
>   Test Summary Report
>   -------------------
>   t1700-split-index.sh                             (Wstat: 256 Tests: 23
>   Failed: 1)
>     Failed test:  23
>     Non-zero exit status: 1
>   Files=809, Tests=18491, 401 wallclock secs ( 7.22 usr  1.60 sys + 263.16
>   cusr 49.58 csys = 321.56 CPU)
>   Result: FAIL
>
> because it can't access the test's verbose log due to lack of
> permissions.
>
>
>   https://travis-ci.org/git/git/jobs/329681826#L2074

I may need help getting that log (or even better the trash directory
of t1700). I tried -m32 build, then valgrind on amd64 (because it
didn't work with my 32 build), running the tests with dash and even
the linux32 docker version that comes with "ci" directory. Everything
worked for me.
-- 
Duy

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Jeff King]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

On Thu, Jan 18, 2018 at 07:47:50PM +0700, Duy Nguyen wrote:

> I may need help getting that log (or even better the trash directory
> of t1700). I tried -m32 build, then valgrind on amd64 (because it
> didn't work with my 32 build), running the tests with dash and even
> the linux32 docker version that comes with "ci" directory. Everything
> worked for me.

It fails for me locally if I run it in the linux32 docker environment.
Here's the end of the "-v -x" output:

  + GIT_INDEX_FILE=/usr/src/git/t/trash directory.t1700-split-index/new-index git -C ro update-index --split-index
  + chmod -R u+w ro
  + rm ro/.git/sharedindex.bbdf63f5a51242904dba2ea929ea4f56fcc340b6
  + GIT_INDEX_FILE=new-index git ls-files
  fatal: .git/sharedindex.bbdf63f5a51242904dba2ea929ea4f56fcc340b6: index file open failed: No such file or directory
  error: last command exited with $?=128
  not ok 23 - graceful handling splitting index is not allowed

I don't know if the trash state will be helpful, but it's attached.

-Peff

[-- Attachment #2: t1700-trash.tar.gz --]
[-- Type: application/gzip, Size: 24979 bytes --]

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Duy Nguyen]

On Thu, Jan 18, 2018 at 8:29 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 18, 2018 at 07:47:50PM +0700, Duy Nguyen wrote:
>
>> I may need help getting that log (or even better the trash directory
>> of t1700). I tried -m32 build, then valgrind on amd64 (because it
>> didn't work with my 32 build), running the tests with dash and even
>> the linux32 docker version that comes with "ci" directory. Everything
>> worked for me.
>
> It fails for me locally if I run it in the linux32 docker environment.
> Here's the end of the "-v -x" output:
>
>   + GIT_INDEX_FILE=/usr/src/git/t/trash directory.t1700-split-index/new-index git -C ro update-index --split-index
>   + chmod -R u+w ro
>   + rm ro/.git/sharedindex.bbdf63f5a51242904dba2ea929ea4f56fcc340b6
>   + GIT_INDEX_FILE=new-index git ls-files
>   fatal: .git/sharedindex.bbdf63f5a51242904dba2ea929ea4f56fcc340b6: index file open failed: No such file or directory
>   error: last command exited with $?=128
>   not ok 23 - graceful handling splitting index is not allowed
>
> I don't know if the trash state will be helpful, but it's attached.

Thanks. Somehow my way of forcing mks_tempfile() to fail, well..
failed in this environment. I see the same output if I remove "chmod
-R u-w ro". I think I have enough to continue from here.
-- 
Duy

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Duy Nguyen]

On Thu, Jan 18, 2018 at 08:36:32PM +0700, Duy Nguyen wrote:
> On Thu, Jan 18, 2018 at 8:29 PM, Jeff King <peff@peff.net> wrote:
> > On Thu, Jan 18, 2018 at 07:47:50PM +0700, Duy Nguyen wrote:
> >
> >> I may need help getting that log (or even better the trash directory
> >> of t1700). I tried -m32 build, then valgrind on amd64 (because it
> >> didn't work with my 32 build), running the tests with dash and even
> >> the linux32 docker version that comes with "ci" directory. Everything
> >> worked for me.
> >
> > It fails for me locally if I run it in the linux32 docker environment.
> > Here's the end of the "-v -x" output:
> >
> >   + GIT_INDEX_FILE=/usr/src/git/t/trash directory.t1700-split-index/new-index git -C ro update-index --split-index
> >   + chmod -R u+w ro
> >   + rm ro/.git/sharedindex.bbdf63f5a51242904dba2ea929ea4f56fcc340b6
> >   + GIT_INDEX_FILE=new-index git ls-files
> >   fatal: .git/sharedindex.bbdf63f5a51242904dba2ea929ea4f56fcc340b6: index file open failed: No such file or directory
> >   error: last command exited with $?=128
> >   not ok 23 - graceful handling splitting index is not allowed
> >
> > I don't know if the trash state will be helpful, but it's attached.
> 
> Thanks. Somehow my way of forcing mks_tempfile() to fail, well..
> failed in this environment. I see the same output if I remove "chmod
> -R u-w ro". I think I have enough to continue from here.

The test suite was run as root, no wonder why my removing write access
has no effect. I got the test to pass with this, but then it fails
with

    Can't write .prove (Permission denied) at /usr/share/perl/5.22/App/Prove.pm line 542.

Some more chown'ing or chmod'ing is required....

-- 8< --
Subject: [PATCH] ci: don't accidentally run the test suite as root

This script assigns and adds a user named "ci" in a subshell so the
outer CI_USER is not affected. For some reason, CI_USER is actually
empty on Travis linux32 builds. This makes the following "su" useless
and the test suite is run as root.

Some tests may expect file/dir permissions to be respected. But root
rules all and ignores all. That could make tests fail.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 ci/run-linux32-build.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index c19c50c1c9..92b488ff27 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -21,8 +21,8 @@ linux32 --32bit i386 sh -c '
 # If a host user id is given, then create a user "ci" with the host user
 # id to make everything accessible to the host user.
 HOST_UID=$1 &&
-CI_USER=$USER &&
-test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) &&
+CI_USER=${USER:-ci} &&
+test -z "$HOST_UID" || useradd -u $HOST_UID $CI_USER &&
 
 # Build and test
 linux32 --32bit i386 su -m -l $CI_USER -c '
-- 
2.16.0.47.g5ff498d35b

-- 8< --

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Jeff King]

On Thu, Jan 18, 2018 at 10:00:14PM +0700, Duy Nguyen wrote:

> The test suite was run as root, no wonder why my removing write access
> has no effect. I got the test to pass with this, but then it fails
> with
> 
>     Can't write .prove (Permission denied) at /usr/share/perl/5.22/App/Prove.pm line 542.
> 
> Some more chown'ing or chmod'ing is required....

Ah, right. I agree that we probably ought to run the ci as non-root.
However, if the test requires non-root, it probably needs to be marked
with the SANITY prereq.

I also ran into one funny thing: if you run the script with "-i", then
we do not run the test_when_finished block. And therefore the "ro"
directory is left without its write bit, and the next test run fails, as
it cannot "rm -rf" the old trash directory out of the way.

I'm not sure there's a good solution, though. Skipping the
test_when_finished block on a "-i" run is intentional, to let you
inspect the broken state.

> Subject: [PATCH] ci: don't accidentally run the test suite as root
> 
> This script assigns and adds a user named "ci" in a subshell so the
> outer CI_USER is not affected. For some reason, CI_USER is actually
> empty on Travis linux32 builds. This makes the following "su" useless
> and the test suite is run as root.

Are we sure this was the problem on Travis, and it wasn't just an issue
with how I reproduced via docker?

-Peff

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[SZEDER Gábor]

On Thu, Jan 18, 2018 at 10:37 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 18, 2018 at 10:00:14PM +0700, Duy Nguyen wrote:
>
>> The test suite was run as root, no wonder why my removing write access
>> has no effect. I got the test to pass with this, but then it fails
>> with
>>
>>     Can't write .prove (Permission denied) at /usr/share/perl/5.22/App/Prove.pm line 542.
>>
>> Some more chown'ing or chmod'ing is required....

This is the fallout of running the tests as root in the past.  With your
patch 'prove' is run as a non-root user, but the prove state is loaded
from Travis CI's cache, where it has been written as root the last time
around, so now we don't have permissions to (over)write it.

I have patches in the works to address this as well.

>> Subject: [PATCH] ci: don't accidentally run the test suite as root
>>
>> This script assigns and adds a user named "ci" in a subshell so the
>> outer CI_USER is not affected. For some reason, CI_USER is actually
>> empty on Travis linux32 builds. This makes the following "su" useless
>> and the test suite is run as root.
>
> Are we sure this was the problem on Travis, and it wasn't just an issue
> with how I reproduced via docker?

Yes, we are.


Gábor

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Duy Nguyen]

On Fri, Jan 19, 2018 at 5:32 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Jan 18, 2018 at 10:37 PM, Jeff King <peff@peff.net> wrote:
>> On Thu, Jan 18, 2018 at 10:00:14PM +0700, Duy Nguyen wrote:
>>
>>> The test suite was run as root, no wonder why my removing write access
>>> has no effect. I got the test to pass with this, but then it fails
>>> with
>>>
>>>     Can't write .prove (Permission denied) at /usr/share/perl/5.22/App/Prove.pm line 542.
>>>
>>> Some more chown'ing or chmod'ing is required....
>
> This is the fallout of running the tests as root in the past.  With your
> patch 'prove' is run as a non-root user, but the prove state is loaded
> from Travis CI's cache, where it has been written as root the last time
> around, so now we don't have permissions to (over)write it.
>
> I have patches in the works to address this as well.

Great. I'll leave it to you then. I will update the test with SANITY
(and a small fix in the test name) and think more about the "-i" Jeff
mentioned.
-- 
Duy

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[SZEDER Gábor]

On Thu, Jan 18, 2018 at 1:47 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Jan 18, 2018 at 6:36 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> This series, queued as 'nd/shared-index-fix', makes the 32 bit Linux
>> build job fail on Travis CI.  Unfortunately, all it can tell us about
>> the failure is this:
>>
>>   Test Summary Report
>>   -------------------
>>   t1700-split-index.sh                             (Wstat: 256 Tests: 23
>>   Failed: 1)
>>     Failed test:  23
>>     Non-zero exit status: 1
>>   Files=809, Tests=18491, 401 wallclock secs ( 7.22 usr  1.60 sys + 263.16
>>   cusr 49.58 csys = 321.56 CPU)
>>   Result: FAIL
>>
>> because it can't access the test's verbose log due to lack of
>> permissions.
>>
>>
>>   https://travis-ci.org/git/git/jobs/329681826#L2074
>
> I may need help getting that log (or even better the trash directory
> of t1700). 

Well, shower thoughts gave me an idea, see the included PoC patch below.
I can't really decide whether it's too clever or too ugly :)

It produces output like this (a previous WIP version without
'--immediate'):

  https://travis-ci.org/szeder/git/jobs/331601009#L2486


  -- >8 --

Subject: [PATCH] travis-ci: include the trash directories of failed tests in
 the trace log

The trash directory of a failed test might contain valuable
information about the cause of the failure, but we have no access to
the trash directories of Travis CI build jobs.  The only feedback we
get from there is the trace log, so...

Modify 'ci/print-test-failures.sh' to create a tar.gz archive of the
test directory of each failed test and encode it with base64, so the
result is a block of ASCII text that can be safely included in the
trace log, along with a hint about how to restore it.  Furthermore,
run tests with '--immediate' to faithfully preserve the failed state.

A few of our tests create a sizeable trash directory, so limit the
size of each included base64-encoded block, let's say, to 1MB.

Note:

  - The logs of Linux build jobs coming from Travis CI have mostly
    CRLF line endings, which makes 'base64 -d' from 'coreutils'
    complain about "invalid input"; it has to be converted to LF
    first.  'openssl base64 -d' can grok it just fine, even without
    conversion.

  - The logs of OSX build jobs have CRCRLF line endings.  However, the
    'base64' util of OSX doesn't wrap its output at 76 columns, i.e.
    prints one single albeit very long line.  This means that while
    'base64' from 'coreutils' still complains, by the time it gets to
    the invalid input it already decoded everything and produced a
    valid .tar.gz.  OTOH, 'openssl base64 -d' doesn't grok it, but
    exits without any error message or even an error code, even after
    converting to CRLF or LF line endings.

    Go figure.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib-travisci.sh        |  2 +-
 ci/print-test-failures.sh | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 1efee55ef7..981c6e9504 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -97,7 +97,7 @@ fi
 export DEVELOPER=1
 export DEFAULT_TEST_TARGET=prove
 export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
-export GIT_TEST_OPTS="--verbose-log"
+export GIT_TEST_OPTS="--verbose-log --immediate"
 export GIT_TEST_CLONE_2GB=YesPlease
 
 case "$jobname" in
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 4f261ddc01..cc6a1247a4 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -23,5 +23,25 @@ do
 		echo "$(tput setaf 1)${TEST_OUT}...$(tput sgr0)"
 		echo "------------------------------------------------------------------------"
 		cat "${TEST_OUT}"
+
+		TEST_NAME="${TEST_EXIT%.exit}"
+		TEST_NAME="${TEST_NAME##*/}"
+		TRASH_DIR="t/trash directory.$TEST_NAME"
+		TRASH_TGZ_B64="$TEST_NAME.trash.base64"
+		if [ -d "$TRASH_DIR" ]
+		then
+			tar czp "$TRASH_DIR" |base64 >"$TRASH_TGZ_B64"
+
+			if [ 1048576 -lt $(wc -c <"$TRASH_TGZ_B64") ]
+			then
+				# larger than 1MB
+				echo "$(tput setaf 1)Trash directory of '$TEST_NAME' is too big to be included in the trace log$(tput sgr0)"
+			else
+				echo "$(tput setaf 1)Trash directory of '$TEST_NAME':$(tput sgr0)"
+				echo "(Extract it to a file from the raw log, make sure it has Unix line endings, then run 'base64 -d <file> |tar vxzp' to restore it)"
+				cat "$TRASH_TGZ_B64"
+				echo "$(tput setaf 1)End of trash directory of '$TEST_NAME'$(tput sgr0)"
+			fi
+		fi
 	fi
 done
-- 
2.16.1.80.gc0eec9753d

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Eric Sunshine]

On Mon, Jan 22, 2018 at 1:27 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Subject: [PATCH] travis-ci: include the trash directories of failed tests in
>  the trace log
>
> The trash directory of a failed test might contain valuable
> information about the cause of the failure, but we have no access to
> the trash directories of Travis CI build jobs.  The only feedback we
> get from there is the trace log, so...
>
> Modify 'ci/print-test-failures.sh' to create a tar.gz archive of the
> test directory of each failed test and encode it with base64, so the
> result is a block of ASCII text that can be safely included in the
> trace log, along with a hint about how to restore it.  Furthermore,
> run tests with '--immediate' to faithfully preserve the failed state.
>
> A few of our tests create a sizeable trash directory, so limit the
> size of each included base64-encoded block, let's say, to 1MB.
>
> Note:
>
>   - The logs of Linux build jobs coming from Travis CI have mostly
>     CRLF line endings, which makes 'base64 -d' from 'coreutils'
>     complain about "invalid input"; it has to be converted to LF
>     first.  'openssl base64 -d' can grok it just fine, even without
>     conversion.
>
>   - The logs of OSX build jobs have CRCRLF line endings.  However, the
>     'base64' util of OSX doesn't wrap its output at 76 columns, i.e.
>     prints one single albeit very long line.  This means that while

Perhaps you could pipe the 'base64' output through 'fold' or 'fmt'?

>     'base64' from 'coreutils' still complains, by the time it gets to
>     the invalid input it already decoded everything and produced a
>     valid .tar.gz.  OTOH, 'openssl base64 -d' doesn't grok it, but
>     exits without any error message or even an error code, even after
>     converting to CRLF or LF line endings.
>
>     Go figure.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[SZEDER Gábor]

On Mon, Jan 22, 2018 at 8:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jan 22, 2018 at 1:27 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

>>   - The logs of OSX build jobs have CRCRLF line endings.  However, the
>>     'base64' util of OSX doesn't wrap its output at 76 columns, i.e.
>>     prints one single albeit very long line.  This means that while
>
> Perhaps you could pipe the 'base64' output through 'fold' or 'fmt'?

No need to, according to its manpage[1], OSX's base64 has an option to
wrap lines after given number of characters.  Of course it uses a
different letter for the option than the coreutils base64... :)

(While long lines are ugly, in this particular case it comes handy: when
using vim to extract the base64-encoded section from the log to a
separate file, it's less keystrokes to yank a single line than to search
for the end of the encoded section.)


[1] - https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/base64.1.html

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Duy Nguyen]

On Tue, Jan 23, 2018 at 1:27 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Thu, Jan 18, 2018 at 1:47 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Thu, Jan 18, 2018 at 6:36 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>> This series, queued as 'nd/shared-index-fix', makes the 32 bit Linux
>>> build job fail on Travis CI.  Unfortunately, all it can tell us about
>>> the failure is this:
>>>
>>>   Test Summary Report
>>>   -------------------
>>>   t1700-split-index.sh                             (Wstat: 256 Tests: 23
>>>   Failed: 1)
>>>     Failed test:  23
>>>     Non-zero exit status: 1
>>>   Files=809, Tests=18491, 401 wallclock secs ( 7.22 usr  1.60 sys + 263.16
>>>   cusr 49.58 csys = 321.56 CPU)
>>>   Result: FAIL
>>>
>>> because it can't access the test's verbose log due to lack of
>>> permissions.
>>>
>>>
>>>   https://travis-ci.org/git/git/jobs/329681826#L2074
>>
>> I may need help getting that log (or even better the trash directory
>> of t1700).
>
> Well, shower thoughts gave me an idea, see the included PoC patch below.
> I can't really decide whether it's too clever or too ugly :)

I can't comment on the patch. But there is one thing I think I should
mention. As someone new to Travis, I didn't know that I could set up
my own Travis jobs and get the logs myself. Maybe you should point
that out when you point people to travis test failures (which I
appreciate).
-- 
Duy

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Lars Schneider]

> On 22 Jan 2018, at 19:27, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 
> 
> On Thu, Jan 18, 2018 at 1:47 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Thu, Jan 18, 2018 at 6:36 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>> This series, queued as 'nd/shared-index-fix', makes the 32 bit Linux
>>> build job fail on Travis CI.  Unfortunately, all it can tell us about
>>> the failure is this:
>>> 
>>>  Test Summary Report
>>>  -------------------
>>>  t1700-split-index.sh                             (Wstat: 256 Tests: 23
>>>  Failed: 1)
>>>    Failed test:  23
>>>    Non-zero exit status: 1
>>>  Files=809, Tests=18491, 401 wallclock secs ( 7.22 usr  1.60 sys + 263.16
>>>  cusr 49.58 csys = 321.56 CPU)
>>>  Result: FAIL
>>> 
>>> because it can't access the test's verbose log due to lack of
>>> permissions.
>>> 
>>> 
>>>  https://travis-ci.org/git/git/jobs/329681826#L2074
>> 
>> I may need help getting that log (or even better the trash directory
>> of t1700). 
> 
> Well, shower thoughts gave me an idea, see the included PoC patch below.
> I can't really decide whether it's too clever or too ugly :)

Creative :-)

Alternatively, we could store write access credentials [1] of a public
repo in Travis and upload the trash directory to it. This could
work "out-of-the-box" for git/git. Personal Travis environments (e.g.
larsxschneider/git) would need to setup that individually.

- Lars

[1] https://docs.travis-ci.com/user/environment-variables/#Defining-Variables-in-Repository-Settings
    ^^^ that's the mechanism we use for the Windows build credentials