| Message ID | 20230423071243.1863977-1-cheskaqiqi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] write-tree: optimize sparse integration | expand |
Shuqi Liang <cheskaqiqi@gmail.com> writes: > 'prepare_repo_settings()' needs to be run inside a repository. Ensure > that the code checks for the presence of a repository before calling > this function. Can you explain why this change is needed? That is, if the caller made sure if this codepath is entered only when inside a repository, such a "we need to refrain from doing this" becomes unnecessary. Describe under what condition the control passes this section with !the_repository->gitdir, e.g. "When the command is run in such and such way outside a repository, the control reaches this position but prepare_repo_settings() cannot be blindly called". I suspect that it is a bug if the control reaches this point without having a repository, as the call to write_index_as_tree() would be already failing if we were not in a repository, but there is no such a bug, and you did not introduce one with your previous changes to this codepath that you need to fix here. You can observe a few things: - "write-tree" in the git.c::commands[] table has RUN_SETUP. - git.c::run_builtin() is repsonsible for calling cmd_write_tree(); what happens before it calls the function? For a command with RUN_SETUP set, unless the command line argument is "-h" (that is, "git write-tree -h" is run), setup_git_directory() is called. - setup_git_directory() dies unless run in a repository. - git.c::run_builtin() calls setup_git_directory_gently() when the command line argument is "-h" and it does not die even run outside a repository. However, before the code you touched, there is a call to parse_options(). - parse_options() called for the command line argument "-h" shows a short help and then exits. So...? Also when starting to talk about totally different things (like, you were discussing the change to write_tree.c to avoid segfaulting when run outside a repository, but now you are going to talk about the title of one test piece), please make sure it is clear for readers. A blank line here may be appropriate. > 'write-tree on all' had an unclear meaning of 'on all'. > Change the test name to simply 'write-tree'. Add a baseline > 'test_all_match git write-tree' before making any changes to the index, > providing a reference point for the 'write-tree' prior to any > modifications. Add a comparison of the output of > 'git status --porcelain=v2' to test the working tree after 'write-tree' > exits. Ensure SKIP_WORKTREE files weren't materialized on disk by using > "test_path_is_missing". All of the above may be easier to read in a bulletted list form, e.g. * 'on all' in the title of the test 'write-tree on all' was unclear; remove it. * test the baseline test_all_match git write-tree" before doing anything else. ... > Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com> > --- > > * Update commit message. OK. > * 'command_requires_full_index' to be set after 'parse_options'. This does not match what we see in this patch. > * Remove trailing whitespace. OK. But there is a new line with a trailing whitespace before the line that says # check that SKIP_WORKTREE files are not materialized" in the test. Thanks.
diff --git a/builtin/write-tree.c b/builtin/write-tree.c index 32e302a813..52caa096a8 100644 --- a/builtin/write-tree.c +++ b/builtin/write-tree.c @@ -41,8 +41,10 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix) argc = parse_options(argc, argv, cmd_prefix, write_tree_options, write_tree_usage, 0); - prepare_repo_settings(the_repository); - the_repository->settings.command_requires_full_index = 0; + if (the_repository->gitdir) { + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + } ret = write_index_as_tree(&oid, &the_index, get_index_file(), flags, tree_prefix); diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 0c784813f1..2a467e4b31 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -2080,22 +2080,32 @@ test_expect_success 'grep sparse directory within submodules' ' test_cmp actual expect ' -test_expect_success 'write-tree on all' ' +test_expect_success 'write-tree' ' init_repos && + test_all_match git write-tree && + write_script edit-contents <<-\EOF && echo text >>"$1" EOF + # make a change inside the sparse cone run_on_all ../edit-contents deep/a && - run_on_all git update-index deep/a && + test_all_match git update-index deep/a && test_all_match git write-tree && + test_all_match git status --porcelain=v2 && + # make a change outside the sparse cone run_on_all mkdir -p folder1 && run_on_all cp a folder1/a && run_on_all ../edit-contents folder1/a && - run_on_all git update-index folder1/a && - test_all_match git write-tree + test_all_match git update-index folder1/a && + test_all_match git write-tree && + test_all_match git status --porcelain=v2 && + + # check that SKIP_WORKTREE files are not materialized + test_path_is_missing sparse-checkout/folder2/a && + test_path_is_missing sparse-index/folder2/a ' test_expect_success 'sparse-index is not expanded: write-tree' '