Re: [PATCH 05/15] connect_work_tree_and_git_dir: safely create leading directories

From: Stefan Beller <sbeller@google.com>
Date: Feb 16, 2017, 21:16 UTC
Message-ID: <CAGZ79kbMmPQ1ZxtdaoFof36g1An-xsAx85etiskirJfY4tGHaQ@mail.gmail.com>
In-Reply-To: <xmqqh93tx3f3.fsf@gitster.mtv.corp.google.com>

On Thu, Feb 16, 2017 at 12:54 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 60 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> In a later patch we'll use connect_work_tree_and_git_dir when the
>> directory for the gitlink file doesn't exist yet. Safely create
>> the directory first.
>>
>> One of the two users of 'connect_work_tree_and_git_dir' already checked
>> for the directory being there, so we can loose that check.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  dir.c       | 32 +++++++++++++++++++++-----------
>>  submodule.c | 11 ++---------
>>  2 files changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 4541f9e146..6f52af7abb 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state *istate,
>>  /* Update gitfile and core.worktree setting to connect work tree and git dir */
>>  void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
>>  {
>> -     struct strbuf file_name = STRBUF_INIT;
>> +     struct strbuf gitfile_sb = STRBUF_INIT;
>> +     struct strbuf cfg_sb = STRBUF_INIT;
>>       struct strbuf rel_path = STRBUF_INIT;
>> -     char *git_dir = real_pathdup(git_dir_);
>> -     char *work_tree = real_pathdup(work_tree_);
>> +     char *git_dir, *work_tree;
>>
>> -     /* Update gitfile */
>> -     strbuf_addf(&file_name, "%s/.git", work_tree);
>> -     write_file(file_name.buf, "gitdir: %s",
>> -                relative_path(git_dir, work_tree, &rel_path));
>> +     /* Prepare .git file */
>> +     strbuf_addf(&gitfile_sb, "%s/.git", work_tree_);
>> +     if (safe_create_leading_directories_const(gitfile_sb.buf))
>> +             die(_("could not create directories for %s"), gitfile_sb.buf);
>> +
>> +     /* Prepare config file */
>> +     strbuf_addf(&cfg_sb, "%s/config", git_dir_);
>> +     if (safe_create_leading_directories_const(cfg_sb.buf))
>> +             die(_("could not create directories for %s"), cfg_sb.buf);
>>
>> +     git_dir = real_pathdup(git_dir_);
>> +     work_tree = real_pathdup(work_tree_);
>> +
>> +     /* Write .git file */
>> +     write_file(gitfile_sb.buf, "gitdir: %s",
>> +                relative_path(git_dir, work_tree, &rel_path));
>
> The above does somewhat more than advertised and was a bit hard to
> grok.  Initially I thought the reason why pathdup()s were delayed
> was perhaps because you pathdup() something potentially different
> from the given parameter to the function (i.e. new code before
> pathdup() may tweak what is pathdup()ed).
>
> But that is not what is happening.  I suspect that you did this to
> avoid leaking allocated memory when the code calls die().

That is not what is happening, either. AFAICT real_pathdup resolves a path to its real path. But the path *must* exist already (except the very last part, i.e. the file name). So the SCLD must come before the real_pathdup, which has to come before its consumers, which are the relative_path calls for write_file as well as git_config_set_in_file.

So structurally we need to:

1a) construct $GIT_DIR/modules/<name>/config 1b) SCLD 1a) 1c) get absolute path of 1a)

2a) construct <submodule path>/.git file 2b) SCLD 2a) 2c) get absolute path of 2a)

3) compute relative path of 1c and 2c
    both ways, write to appropriate places.

I chose to structure it as:

1a 1b 2a 2b

1c 2c 3

because the a/b steps seemed very related and the order is valid.

Show 7 quoted lines

>
> If the code was written like so from the beginning, I do not see a
> reason to move the pathdup() up to deliberately make it leak [*1*].
> But as a part of this change, I found it distracting and getting in
> the way of understanding the change.  If you really care, it is
> nicer to do it to reviewers as a separate preparatory clean-up step,
> or follow-up standalone clean-up patch after the series settles.

I considered doing it in multiple patches as we have different things
going on here:
1) as the commit claims have SCLD in connect_work_tree_and_git_dir
2) keep the dependencies of SCLD and real_pathdup in order.  C.f.
  the deleted lines of code in submodule.c

Show 5 quoted lines

> It is a good thing to do to make sure git_dir_ exists and it should
> be mentioned in the log message.  Adding "Do the same for the place
> where the per-repo config file is created". at the end of the first
> paragraph should be sufficient.
>

will do.

Thanks for thorough review! Stefan