Junio C Hamano <gitster@xxxxxxxxx> writes:
> By the way, this line is the last use of sm_gitdir_rel before it
> gets freed. I wonder
>
> sm_gitdir = absolute_path(sb.buf);
>
> would be sufficient. We can lose the variable, and free() on it at
> the end of this function, and an extra allocation if we can do so.
>
>> + if (!is_absolute_path(path)) {
>> + strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path);
>> + path = strbuf_detach(&sb, 0);
>> + } else
>> + path = xstrdup(path);
>>
>> if (!file_exists(sm_gitdir)) {
>> if (safe_create_leading_directories_const(sm_gitdir) < 0)
>
> Other than that, looks good to me.
Another thing I noticed is that it will not stay safe forever to
borrow the result from absolute_path() for extended period of time.
So how about this one (and Ramsay's "NULL must be spelled NULL, not
0") on top?
-- >8 --
From: Junio C Hamano <gitster@xxxxxxxxx>
Date: Fri, 1 Apr 2016 12:23:16 -0700
Subject: [PATCH] submodule--helper: do not borrow absolute_path() result for too long
absolute_path() is designed to allow its callers to take a brief
peek of the result (typically, to be fed to functions like
strbuf_add() and relative_path() as a parameter) without having to
worry about freeing it, but the other side of the coin of that
memory model is that the caller shouldn't rely too much on the
result living forever--there may be a helper function the caller
subsequently calls that makes its own call to absolute_path(),
invalidating the earlier result.
Use xstrdup() to make our own copy, and free(3) it when we are done.
While at it, remove an unnecessary sm_gitdir_rel variable that was
only used to as a parameter to call absolute_paht() and never used
again.
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
builtin/submodule--helper.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b660a22..e69b340 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -157,8 +157,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
const char *reference = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
- char *sm_gitdir_rel, *p, *path = NULL;
- const char *sm_gitdir;
+ char *p, *path = NULL, *sm_gitdir;
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
@@ -199,8 +198,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
die(_("submodule--helper: unspecified or empty --path"));
strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
- sm_gitdir_rel = strbuf_detach(&sb, NULL);
- sm_gitdir = absolute_path(sm_gitdir_rel);
+ sm_gitdir = xstrdup(absolute_path(sb.buf));
if (!is_absolute_path(path)) {
strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path);
@@ -245,7 +243,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
relative_path(path, sm_gitdir, &rel_path));
strbuf_release(&sb);
strbuf_release(&rel_path);
- free(sm_gitdir_rel);
+ free(sm_gitdir);
free(path);
free(p);
return 0;
--
2.8.0-225-g297c00e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html