Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

Junio C Hamano <gitster@pobox.com> writes:

That would look something like this squashed into 4/3, I think. I am not sure if a commit that squashes 2/3, 3/3, 4/3 and this update together is harder to understand than keeping 2/3, 3/3 and a fixed 4/3 separate, though. The end result looks much better structured than before 2/3 is applied to my quick scan-through of the code.

In any case, the log message of 2/3 needs to be updated to retitle it, I think. "do not ... more often than necessary" makes it sound as if we were doing things that did not make any difference in the end result, wasting cycles. But what you actually wanted to achieve was not to "avoid unnecessary work"--doing so gave a broken behaviour and that was what you were fixing, "do not record broken core.worktree", perhaps?

The solution (if we squash 2-4 and the fixup below) is to stop feeding get_git_dir() to needs_work_tree_config(), because the parameter to the latter is the path to ".git" that presumably is at the top of the working tree, and get_git_dir() is not that when "gitdir" file is involved. So a rewritten log message may say something like...

    init: do not set unnecessary core.worktree

    The function needs_work_tree_config() that is called from
    create_default_files() is supposed to be fed the path to ".git"
    that looks as if it is at the top of the working tree, and
    decide if that location matches the actual worktree being used.
    This comparison allows "git init" to decide if core.worktree
    needs to be recorded in the working tree.

    In the current code, however, we feed the return value from
    get_git_dir(), which can be totally different from what the
    function expects when "gitdir" file is involved.  Instead of
    giving the path to the ".git" at the top of the working tree, we
    end up feeding the actual path that the file points at.

    This original location of ".git" however is only known to a
    helper function set_git_dir_init() that must be called before
    init_db() is called (they both have only two callsites, one in
    "git init" and the other in "git clone"), and in the current
    code, this original location is not visible to its callers.

    By doing the following two things:

    * Move call to set_git_dir_init() to init_db(), as the two must
      always be called in this order, and adjust its current
      callers.

    * Make set_git_dir_init() return the original location of ".git"
      to the caller, which is init_db(), and have it passed to
      create_default_files() as a new parameter.

   pass the correct location down to needs_work_tree_config() to fix
   this.

This suggests that 2/3, 3/3 and fixed 4/3 could be done in two logical steps. The first bullet point can be done as a separate preparatory step, and on top of that, the second bullet point can be done as a separate "fix".

 builtin/init-db.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)