On Thu, Feb 16, 2017 at 12:54 PM, Junio C Hamano <[email protected]> wrote:
> Stefan Beller <[email protected]> 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 <[email protected]>
>> ---
>> 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.
>
> 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
> 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
