Shuqi Liang <cheskaqiqi@xxxxxxxxx> 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@xxxxxxxxx> > --- > > * 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.
