Re: [PATCH v11 0/8] submodule inline diff format
On Wed, Aug 31, 2016 at 9:45 AM, Junio C Hamano wrote: > Jacob Keller writes: > >>> We probably should release for the error case. I'll do that. I don't >>> believe do_submodule_path ensures that the passed in argument is >>> guaranteed to not be initialized or used. >>> >>> Thanks, >>> Jake >> >> Here's the squash for this fix. > > It seems that the topic has been quiet since the message I am > responding to. Have all the issues been resolved, or are there > still some loose ends to be hashed out? > > Hoping that the answer is "the former", I'll mark this topic as > "Waiting for a reroll." > > Thanks. As far as I know they have. You mentioned a few nits in one comment, but then said you had a re-worded message that you queued so I assumed those nits were all squashed in. If that's correct, and you already applied my squash for the strbuf leak I think we're good and I had nothing coming in a future re-roll based on looking at what you have queued in your jk/diff-submodule-inline-diff branch. Regards, Jake
Re: [PATCH v11 0/8] submodule inline diff format
Jacob Keller writes: >> We probably should release for the error case. I'll do that. I don't >> believe do_submodule_path ensures that the passed in argument is >> guaranteed to not be initialized or used. >> >> Thanks, >> Jake > > Here's the squash for this fix. It seems that the topic has been quiet since the message I am responding to. Have all the issues been resolved, or are there still some loose ends to be hashed out? Hoping that the answer is "the former", I'll mark this topic as "Waiting for a reroll." Thanks.
Re: [PATCH v11 0/8] submodule inline diff format
From: Jacob Keller
> On Fri, Aug 26, 2016 at 1:04 PM, Jeff King wrote:
> > On Fri, Aug 26, 2016 at 07:58:07PM +, Keller, Jacob E wrote:
> >
> >> > > char *git_pathdup_submodule(const char *path, const char *fmt,
> >> > > ...)
> >> > > {
> >> > > + int err;
> >> > > va_list args;
> >> > > struct strbuf buf = STRBUF_INIT;
> >> > > va_start(args, fmt);
> >> > > - do_submodule_path(&buf, path, fmt, args);
> >> > > + err = do_submodule_path(&buf, path, fmt, args);
> >> > > va_end(args);
> >> > > + if (err)
> >> >
> >> > Here we need a strbuf_release(&buf) to avoid a memory leak?
> >>
> >> No, cause we "strbuf_detach" after this to return the buffer? Or is
> >> that not safe?
> >
> > That code path is OK. I think the question is whether you need to
> > release the buffer in the "err" case where you return NULL and don't hit
> > the strbuf_detach.
> >
> > IOW, does do_submodule_path() promise that when it returns an error,
> > "buf" has been left uninitialized? Some of our strbuf functions do, but
> > I do not know offhand about do_submodule_path().
> >
> > -Peff
>
> We probably should release for the error case. I'll do that. I don't
> believe do_submodule_path ensures that the passed in argument is
> guaranteed to not be initialized or used.
>
> Thanks,
> Jake
Here's the squash for this fix.
Thanks,
Jake
-->8
>From Jacob Keller
>From 9cf89634e6f2b0f3f90f43a553f55eb57bb2f662 Mon Sep 17 00:00:00 2001
From: Jacob Keller
Date: Fri, 26 Aug 2016 16:06:54 -0700
Subject: [PATCH] squash! allow do_submodule_path to work even if submodule
isn't checked out
Add a missing strbuf_release() when returning during the error flow of
git_pathdup_submodule().
Signed-off-by: Jacob Keller
---
path.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/path.c b/path.c
index e9369f75319d..b8515973252c 100644
--- a/path.c
+++ b/path.c
@@ -525,8 +525,10 @@ char *git_pathdup_submodule(const char *path, const char
*fmt, ...)
va_start(args, fmt);
err = do_submodule_path(&buf, path, fmt, args);
va_end(args);
- if (err)
+ if (err) {
+ strbuf_release(&buf);
return NULL;
+ }
return strbuf_detach(&buf, NULL);
}
--
2.10.0.rc0.259.g83512d9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 0/8] submodule inline diff format
On Fri, Aug 26, 2016 at 1:04 PM, Jeff King wrote:
> On Fri, Aug 26, 2016 at 07:58:07PM +, Keller, Jacob E wrote:
>
>> > > char *git_pathdup_submodule(const char *path, const char *fmt,
>> > > ...)
>> > > {
>> > > + int err;
>> > > va_list args;
>> > > struct strbuf buf = STRBUF_INIT;
>> > > va_start(args, fmt);
>> > > - do_submodule_path(&buf, path, fmt, args);
>> > > + err = do_submodule_path(&buf, path, fmt, args);
>> > > va_end(args);
>> > > + if (err)
>> >
>> > Here we need a strbuf_release(&buf) to avoid a memory leak?
>>
>> No, cause we "strbuf_detach" after this to return the buffer? Or is
>> that not safe?
>
> That code path is OK. I think the question is whether you need to
> release the buffer in the "err" case where you return NULL and don't hit
> the strbuf_detach.
>
> IOW, does do_submodule_path() promise that when it returns an error,
> "buf" has been left uninitialized? Some of our strbuf functions do, but
> I do not know offhand about do_submodule_path().
>
> -Peff
We probably should release for the error case. I'll do that. I don't
believe do_submodule_path ensures that the passed in argument is
guaranteed to not be initialized or used.
Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 0/8] submodule inline diff format
On Fri, Aug 26, 2016 at 07:58:07PM +, Keller, Jacob E wrote:
> > > char *git_pathdup_submodule(const char *path, const char *fmt,
> > > ...)
> > > {
> > > + int err;
> > > va_list args;
> > > struct strbuf buf = STRBUF_INIT;
> > > va_start(args, fmt);
> > > - do_submodule_path(&buf, path, fmt, args);
> > > + err = do_submodule_path(&buf, path, fmt, args);
> > > va_end(args);
> > > + if (err)
> >
> > Here we need a strbuf_release(&buf) to avoid a memory leak?
>
> No, cause we "strbuf_detach" after this to return the buffer? Or is
> that not safe?
That code path is OK. I think the question is whether you need to
release the buffer in the "err" case where you return NULL and don't hit
the strbuf_detach.
IOW, does do_submodule_path() promise that when it returns an error,
"buf" has been left uninitialized? Some of our strbuf functions do, but
I do not know offhand about do_submodule_path().
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 0/8] submodule inline diff format
On Fri, 2016-08-26 at 12:17 -0700, Stefan Beller wrote:
> On Thu, Aug 25, 2016 at 4:32 PM, Jacob Keller om> wrote:
>
> >
> > @@ -487,12 +490,14 @@ static void do_submodule_path(struct strbuf
> > *buf, const char *path,
> > strbuf_addstr(buf, git_dir);
> > }
> > if (!is_git_directory(buf->buf)) {
> > + gitmodules_config();
>
> We determined via chat that calling gitmodules_config
> is not harmful w.r.t. correctness, but might require some
> improvements in the future for performance.
> (i.e. we may want to add in a later series a
> if (already called gitmodules_config)
> set flag "already called gitmodules_config";
> return;
> into gitmodules_config)
>
> >
> >
> > char *git_pathdup_submodule(const char *path, const char *fmt,
> > ...)
> > {
> > + int err;
> > va_list args;
> > struct strbuf buf = STRBUF_INIT;
> > va_start(args, fmt);
> > - do_submodule_path(&buf, path, fmt, args);
> > + err = do_submodule_path(&buf, path, fmt, args);
> > va_end(args);
> > + if (err)
>
> Here we need a strbuf_release(&buf) to avoid a memory leak?
No, cause we "strbuf_detach" after this to return the buffer? Or is
that not safe?
Thanks,
Jake
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: [PATCH v11 0/8] submodule inline diff format
On Thu, Aug 25, 2016 at 4:32 PM, Jacob Keller wrote:
> @@ -487,12 +490,14 @@ static void do_submodule_path(struct strbuf *buf, const
> char *path,
> strbuf_addstr(buf, git_dir);
> }
> if (!is_git_directory(buf->buf)) {
> + gitmodules_config();
We determined via chat that calling gitmodules_config
is not harmful w.r.t. correctness, but might require some
improvements in the future for performance.
(i.e. we may want to add in a later series a
if (already called gitmodules_config)
set flag "already called gitmodules_config";
return;
into gitmodules_config)
>
> char *git_pathdup_submodule(const char *path, const char *fmt, ...)
> {
> + int err;
> va_list args;
> struct strbuf buf = STRBUF_INIT;
> va_start(args, fmt);
> - do_submodule_path(&buf, path, fmt, args);
> + err = do_submodule_path(&buf, path, fmt, args);
> va_end(args);
> + if (err)
Here we need a strbuf_release(&buf) to avoid a memory leak?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v11 0/8] submodule inline diff format
From: Jacob Keller
Modify the changes to do_submodule_path so that we properly call
gitmodules_config() before the lookup of submodule_from_path. This may
need to be modified so that we only call it the first time as I'm not
sure what sort of performance hit we'll see. Note that we only need this
call if we no longer have the checkout so it may not be so bad.
Also make the function propagate an error code up to the callers so that
they don't try to use an invalid path. This is better than die()'ing
because a failure can occur if the submodule configuration can't be
found for some reason (such as committing the submodule but not the
.gitmodules file which we previously did in the test).
interdiff between v10 and v11:
diff --git w/cache.h c/cache.h
index 70428e92d7ed..4f6693afa387 100644
--- w/cache.h
+++ c/cache.h
@@ -819,8 +819,8 @@ extern void strbuf_git_common_path(struct strbuf *sb, const
char *fmt, ...)
__attribute__((format (printf, 2, 3)));
extern char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
-extern void strbuf_git_path_submodule(struct strbuf *sb, const char *path,
- const char *fmt, ...)
+extern int strbuf_git_path_submodule(struct strbuf *sb, const char *path,
+const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
diff --git w/path.c c/path.c
index 07dd0f62eb82..e9369f75319d 100644
--- w/path.c
+++ c/path.c
@@ -469,13 +469,16 @@ const char *worktree_git_path(const struct worktree *wt,
const char *fmt, ...)
return pathname->buf;
}
-static void do_submodule_path(struct strbuf *buf, const char *path,
- const char *fmt, va_list args)
+/* Returns 0 on success, non-zero on failure. */
+#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
+static int do_submodule_path(struct strbuf *buf, const char *path,
+const char *fmt, va_list args)
{
const char *git_dir;
struct strbuf git_submodule_common_dir = STRBUF_INIT;
struct strbuf git_submodule_dir = STRBUF_INIT;
const struct submodule *sub;
+ int err = 0;
strbuf_addstr(buf, path);
strbuf_complete(buf, '/');
@@ -487,12 +490,14 @@ static void do_submodule_path(struct strbuf *buf, const
char *path,
strbuf_addstr(buf, git_dir);
}
if (!is_git_directory(buf->buf)) {
+ gitmodules_config();
sub = submodule_from_path(null_sha1, path);
- if (sub) {
- strbuf_reset(buf);
- strbuf_git_path(buf, "%s/%s", "modules",
- sub->name);
+ if (!sub) {
+ err = SUBMODULE_PATH_ERR_NOT_CONFIGURED;
+ goto cleanup;
}
+ strbuf_reset(buf);
+ strbuf_git_path(buf, "%s/%s", "modules", sub->name);
}
strbuf_addch(buf, '/');
@@ -505,27 +510,36 @@ static void do_submodule_path(struct strbuf *buf, const
char *path,
strbuf_cleanup_path(buf);
+cleanup:
strbuf_release(&git_submodule_dir);
strbuf_release(&git_submodule_common_dir);
+
+ return err;
}
char *git_pathdup_submodule(const char *path, const char *fmt, ...)
{
+ int err;
va_list args;
struct strbuf buf = STRBUF_INIT;
va_start(args, fmt);
- do_submodule_path(&buf, path, fmt, args);
+ err = do_submodule_path(&buf, path, fmt, args);
va_end(args);
+ if (err)
+ return NULL;
return strbuf_detach(&buf, NULL);
}
-void strbuf_git_path_submodule(struct strbuf *buf, const char *path,
- const char *fmt, ...)
+int strbuf_git_path_submodule(struct strbuf *buf, const char *path,
+ const char *fmt, ...)
{
+ int err;
va_list args;
va_start(args, fmt);
- do_submodule_path(buf, path, fmt, args);
+ err = do_submodule_path(buf, path, fmt, args);
va_end(args);
+
+ return err;
}
static void do_git_common_path(struct strbuf *buf,
diff --git w/refs/files-backend.c c/refs/files-backend.c
index 12290d249643..1f34b444af8d 100644
--- w/refs/files-backend.c
+++ c/refs/files-backend.c
@@ -1225,13 +1225,19 @@ static void read_loose_refs(const char *dirname, struct
ref_dir *dir)
struct strbuf refname;
struct strbuf path = STRBUF_INIT;
size_t path_baselen;
+ int err = 0;
if (*refs->name)
- strbuf_git_path_submodule(&path, refs->name, "%s", dirname);
+ err = strbuf_git_path_submodule(&path, refs->name, "%s",
dirname);
else
strbuf_git_path(&path, "%s", dirname);
path_baselen = path.len;
+ if (err)
