On Thu, Feb 16, 2017 at 12:54 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> 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@xxxxxxxxxx> >> --- >> 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