Re: [PATCH v5] write-tree: optimize sparse integration

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2023, 16:00 UTC
Message-ID: <xmqqleihgtho.fsf@gitster.g>
In-Reply-To: <20230423071243.1863977-1-cheskaqiqi@gmail.com>

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.

Show 8 quoted lines

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