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

[Junio C Hamano <gitster@pobox.com>]

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().

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.

The comment "prepare config file" was misleading; it is preparing
the place the config file will be created (same for "prepare .git
file").

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.

Thanks.


[Footnote]

*1* it is arguable a small piece of unfreed memory before exit() or
    die() is worth called "leak", though.