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

[Junio C Hamano <gitster@pobox.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.

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