nd/shared-index-fix update
cover nd/shared-index-fix update
This only changes the last patch to correct the test prerequisite and a couple minor refinements. Interdiff:
t/t1700-split-index.sh +4 -4
@@ -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(-)
Signature · 1 line
2.16.0.47.g3d9b0fac3a
PATCH 1/3 read-cache.c: change type of "temp" in write_shared_index()
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.
--- read-cache.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
read-cache.c +11 -9
@@ -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);
Signature · 1 line
2.16.0.47.g3d9b0fac3a
PATCH 2/3 read-cache.c: move tempfile creation/cleanup out of write_shared_index
In reply to Nguyễn Thái Ngọc DuyFor 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().
--- read-cache.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
read-cache.c +17 -16
@@ -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; }
Signature · 1 line
2.16.0.47.g3d9b0fac3a
PATCH 3/3 read-cache: don't write index twice if we can't write shared index
In reply to Nguyễn Thái Ngọc DuyIn 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]
--- read-cache.c | 5 +++-- t/t1700-split-index.sh | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-)
read-cache.c +3 -2
@@ -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))
t/t1700-split-index.sh +19 -0
@@ -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
Signature · 1 line
2.16.0.47.g3d9b0fac3a
Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index
t/t1700-split-index.sh done <<\EOF · line 404
Quoted 9 lines · Nguyễn Thái Ngọc Duy
t/t1700-split-index.sh +1
-0@@ -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.
Quoted 19 lines
t/t1700-split-index.sh +18
-0+ 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
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Nguyễn Thái Ngọc Duy
t/t1700-split-index.sh +0
-0@@ -401,4 +401,23 @@ done <<\EOF 0642 -rw-r---w- EOF
Quoted 6 lines
t/t1700-split-index.sh done <<\EOF · line 404
Quoted 1 line
t/t1700-split-index.sh +1
-0+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.
Quoted 20 lines
Quoted 20 lines
t/t1700-split-index.sh +18
-0+ 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
Signature · 1 line
Duy
cover nd/shared-index-fix updates
In reply to Junio C HamanoThis 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(-)
Signature · 1 line
2.16.0.47.g3d9b0fac3a
PATCH 1/3 read-cache.c: change type of "temp" in write_shared_index()
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.
--- read-cache.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)
read-cache.c +11 -9
@@ -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);
Signature · 1 line
2.16.0.47.g3d9b0fac3a
PATCH 2/3 read-cache.c: move tempfile creation/cleanup out of write_shared_index
In reply to Nguyễn Thái Ngọc DuyFor 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().
--- read-cache.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
read-cache.c +17 -16
@@ -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; }
Signature · 1 line
2.16.0.47.g3d9b0fac3a
PATCH 3/3 read-cache: don't write index twice if we can't write shared index
In reply to Nguyễn Thái Ngọc DuyIn 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]
--- read-cache.c | 5 +++-- t/t1700-split-index.sh | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-)
read-cache.c +3 -2
@@ -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))
t/t1700-split-index.sh +19 -0
@@ -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
Signature · 1 line
2.16.0.47.g3d9b0fac3a
Re: [PATCH 0/3] nd/shared-index-fix updates
In reply to Nguyễn Thái Ngọc DuyQuoted 4 lines · 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.
Quoted 8 lines
Nguyễn Thái Ngọc Duy (3):
read-cache.c: change type of "temp" inwrite_shared_index()read-cache.c: move tempfile creation/cleanup out ofwrite_shared_index
read-cache: don't write index twice if we can't write shared indexread-cache.c | 40 ++++++++++++++++++++++------------------ t/t1700-split-index.sh | 19 +++++++++++++++++++ 2 files changed, 41 insertions(+), 18 deletions(-)
Thanks; let's move this to 'next'.