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

[Stefan Beller]

In a later patch we'll use connect_work_tree_and_git_dir when the
directory for the gitlink file doesn't exist yet. This patch makes
connect_work_tree_and_git_dir safe to use for both cases of
either the git dir or the working dir missing.

To do so, we need to call safe_create_leading_directories[_const]
on both directories. However this has to happen before we construct
the absolute paths as real_pathdup assumes the directories to
be there already.

So for both the config file in the git dir as well as the .git link
file we need to
a) construct the name
b) call SCLD
c) get the absolute path
d) once a-c is done for both we can consume the absolute path
   to compute the relative path to each other and store those
   relative paths.

The implementation provided here puts a) and b) for both cases first,
and then performs c and d after.

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 as
connect_work_tree_and_git_dir handles this functionality now.

Signed-off-by: Stefan Beller 
---
 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(_name, "%s/.git", work_tree);
-   write_file(file_name.buf, "gitdir: %s",
-  relative_path(git_dir, work_tree, _path));
+   /* Prepare .git file */
+   strbuf_addf(_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(_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, _path));
/* Update core.worktree setting */
-   strbuf_reset(_name);
-   strbuf_addf(_name, "%s/config", git_dir);
-   git_config_set_in_file(file_name.buf, "core.worktree",
+   git_config_set_in_file(cfg_sb.buf, "core.worktree",
   relative_path(work_tree, git_dir, _path));
 
-   strbuf_release(_name);
+   strbuf_release(_sb);
+   strbuf_release(_sb);
strbuf_release(_path);
free(work_tree);
free(git_dir);
diff --git a/submodule.c b/submodule.c
index 0e55372f37..04d185738f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1442,8 +1442,6 @@ void absorb_git_dir_into_superproject(const char *prefix,
 
/* Not populated? */
if (!sub_git_dir) {
-   char *real_new_git_dir;
-   const char *new_git_dir;
const struct submodule *sub;
 
if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
@@ -1466,13 +1464,8 @@ void absorb_git_dir_into_superproject(const char *prefix,
sub = submodule_from_path(null_sha1, path);
if (!sub)
die(_("could not lookup name for submodule '%s'"), 
path);
-   new_git_dir = git_path("modules/%s", sub->name);
-   if (safe_create_leading_directories_const(new_git_dir) < 0)
-   die(_("could not create directory '%s'"), new_git_dir);
-   real_new_git_dir = real_pathdup(new_git_dir);
-   connect_work_tree_and_git_dir(path, real_new_git_dir);
-
-   free(real_new_git_dir);
+   connect_work_tree_and_git_dir(path,
+   git_path("modules/%s", sub->name));
} else {
/* Is it already absorbed into the superprojects git dir? */
char *real_sub_git_dir = real_pathdup(sub_git_dir);
-- 
2.12.0.rc1.16.ge4278d41a0.dirty

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

[Junio C Hamano]

Stefan Beller  writes:

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

That's a good sign that you need a bit more in the log message.

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

[Stefan Beller]

On Thu, Feb 16, 2017 at 12:54 PM, Junio C Hamano  wrote:
> Stefan Beller  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 
>> ---
>>  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(_name, "%s/.git", work_tree);
>> - write_file(file_name.buf, "gitdir: %s",
>> -relative_path(git_dir, work_tree, _path));
>> + /* Prepare .git file */
>> + strbuf_addf(_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(_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, _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//config
1b) SCLD 1a)
1c) get absolute path of 1a)

2a) construct /.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

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

[Junio C Hamano]

Stefan Beller  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 
> ---
>  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(_name, "%s/.git", work_tree);
> - write_file(file_name.buf, "gitdir: %s",
> -relative_path(git_dir, work_tree, _path));
> + /* Prepare .git file */
> + strbuf_addf(_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(_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, _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.

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

[Stefan Beller]

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 
---
 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(_name, "%s/.git", work_tree);
-   write_file(file_name.buf, "gitdir: %s",
-  relative_path(git_dir, work_tree, _path));
+   /* Prepare .git file */
+   strbuf_addf(_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(_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, _path));
/* Update core.worktree setting */
-   strbuf_reset(_name);
-   strbuf_addf(_name, "%s/config", git_dir);
-   git_config_set_in_file(file_name.buf, "core.worktree",
+   git_config_set_in_file(cfg_sb.buf, "core.worktree",
   relative_path(work_tree, git_dir, _path));
 
-   strbuf_release(_name);
+   strbuf_release(_sb);
+   strbuf_release(_sb);
strbuf_release(_path);
free(work_tree);
free(git_dir);
diff --git a/submodule.c b/submodule.c
index 0e55372f37..04d185738f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1442,8 +1442,6 @@ void absorb_git_dir_into_superproject(const char *prefix,
 
/* Not populated? */
if (!sub_git_dir) {
-   char *real_new_git_dir;
-   const char *new_git_dir;
const struct submodule *sub;
 
if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
@@ -1466,13 +1464,8 @@ void absorb_git_dir_into_superproject(const char *prefix,
sub = submodule_from_path(null_sha1, path);
if (!sub)
die(_("could not lookup name for submodule '%s'"), 
path);
-   new_git_dir = git_path("modules/%s", sub->name);
-   if (safe_create_leading_directories_const(new_git_dir) < 0)
-   die(_("could not create directory '%s'"), new_git_dir);
-   real_new_git_dir = real_pathdup(new_git_dir);
-   connect_work_tree_and_git_dir(path, real_new_git_dir);
-
-   free(real_new_git_dir);
+   connect_work_tree_and_git_dir(path,
+   git_path("modules/%s", sub->name));
} else {
/* Is it already absorbed into the superprojects git dir? */
char *real_sub_git_dir = real_pathdup(sub_git_dir);
-- 
2.12.0.rc1.16.ge4278d41a0.dirty