[PATCH 0/3] Fix some documented fixmes

[PATCH 0/3] Fix some documented fixmes

[Jared Hance]

A few patches that (hopefully) don't change the behavior of git except to
rectify a memory error. Also, this should ever so slightly help with
the parallelism of git (a GSoC proposal). All of these were found with
commented FIXME and git grep.

Jared Hance (3):
  Use startup_info->prefix rather than prefix.
  Fix memory leak in apply_patch in apply.c.
  Add threaded versions of functions in symlinks.c.

 builtin/apply.c |   10 ++++++++--
 cache.h         |    4 +++-
 git.c           |    2 +-
 symlinks.c      |   28 ++++++++++++++++++++++++++--
 trace.c         |   10 +++++-----
 5 files changed, 43 insertions(+), 11 deletions(-)

-- 
1.7.3.4

[PATCH 1/3] Use startup_info->prefix rather than prefix.

[Jared Hance]

In trace_repo_setup, prefix is passed in as startup_info->prefix. But, as
indicated but a FIXME comment, trace_repo_setup has access to
startup_info. The prefix parameter has therefor been eliminated.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 cache.h |    2 +-
 git.c   |    2 +-
 trace.c |   10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index e12b15f..baa8852 100644
--- a/cache.h
+++ b/cache.h
@@ -1211,7 +1211,7 @@ extern void trace_printf(const char *format, ...);
 extern void trace_vprintf(const char *key, const char *format, va_list ap);
 __attribute__((format (printf, 2, 3)))
 extern void trace_argv_printf(const char **argv, const char *format, ...);
-extern void trace_repo_setup(const char *prefix);
+extern void trace_repo_setup(void);
 extern int trace_want(const char *key);
 extern void trace_strbuf(const char *key, const struct strbuf *buf);
 
diff --git a/git.c b/git.c
index 3805616..7dcc527 100644
--- a/git.c
+++ b/git.c
@@ -296,7 +296,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
 		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
-			trace_repo_setup(prefix);
+			trace_repo_setup();
 	}
 	commit_pager_choice();
 
diff --git a/trace.c b/trace.c
index d953416..ebacf24 100644
--- a/trace.c
+++ b/trace.c
@@ -152,8 +152,7 @@ static const char *quote_crnl(const char *path)
 	return new_path;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
+void trace_repo_setup(void)
 {
 	static const char *key = "GIT_TRACE_SETUP";
 	const char *git_work_tree;
@@ -168,13 +167,14 @@ void trace_repo_setup(const char *prefix)
 	if (!(git_work_tree = get_git_work_tree()))
 		git_work_tree = "(null)";
 
-	if (!prefix)
-		prefix = "(null)";
+	if (!startup_info->prefix)
+		startup_info->prefix = "(null)";
 
 	trace_printf_key(key, "setup: git_dir: %s\n", quote_crnl(get_git_dir()));
 	trace_printf_key(key, "setup: worktree: %s\n", quote_crnl(git_work_tree));
 	trace_printf_key(key, "setup: cwd: %s\n", quote_crnl(cwd));
-	trace_printf_key(key, "setup: prefix: %s\n", quote_crnl(prefix));
+	trace_printf_key(key, "setup: prefix: %s\n", 
+			 quote_crnl(startup_info->prefix));
 }
 
 int trace_want(const char *key)
-- 
1.7.3.4

Re: [PATCH 1/3] Use startup_info->prefix rather than prefix.

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

> In trace_repo_setup, prefix is passed in as startup_info->prefix. But, as
> indicated but a FIXME comment, trace_repo_setup has access to
> startup_info. The prefix parameter has therefor been eliminated.
>
> Signed-off-by: Jared Hance <jaredhance@gmail.com>
> ---

This comes from a9ca8a8 (builtins: print setup info if repo is found,
2010-11-26) and hasn't ever changed over time, even across f07d6a1 (setup:
save prefix (original cwd relative to toplevel) in startup_info,
2010-12-01) that did add the necessary "prefix" field to the startup_info
and was done reasonably close to the patch that wanted to have the field
in the first place.

The fix looks too easy to be correct X-<; in other words, I find it hard
to believe that such a triviality was left without a good reason, but I do
not think of any.

Well, but perhaps something too good to be true is indeed true sometimes.

>  cache.h |    2 +-
>  git.c   |    2 +-
>  trace.c |   10 +++++-----
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index e12b15f..baa8852 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1211,7 +1211,7 @@ extern void trace_printf(const char *format, ...);
>  extern void trace_vprintf(const char *key, const char *format, va_list ap);
>  __attribute__((format (printf, 2, 3)))
>  extern void trace_argv_printf(const char **argv, const char *format, ...);
> -extern void trace_repo_setup(const char *prefix);
> +extern void trace_repo_setup(void);
>  extern int trace_want(const char *key);
>  extern void trace_strbuf(const char *key, const struct strbuf *buf);
>  
> diff --git a/git.c b/git.c
> index 3805616..7dcc527 100644
> --- a/git.c
> +++ b/git.c
> @@ -296,7 +296,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  
>  		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
>  		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
> -			trace_repo_setup(prefix);
> +			trace_repo_setup();
>  	}
>  	commit_pager_choice();
>  
> diff --git a/trace.c b/trace.c index d953416..ebacf24 100644 ---
> a/trace.c +++ b/trace.c @@ -152,8 +152,7 @@ static const char
> *quote_crnl(const char *path) return new_path;
>  }
>  
> -/* FIXME: move prefix to startup_info struct and get rid of this arg */
> -void trace_repo_setup(const char *prefix)
> +void trace_repo_setup(void)
>  {
>  	static const char *key = "GIT_TRACE_SETUP";
>  	const char *git_work_tree;
> @@ -168,13 +167,14 @@ void trace_repo_setup(const char *prefix)
>  	if (!(git_work_tree = get_git_work_tree()))
>  		git_work_tree = "(null)";
>  
> -	if (!prefix)
> -		prefix = "(null)";
> +	if (!startup_info->prefix)
> +		startup_info->prefix = "(null)";
>  
>  	trace_printf_key(key, "setup: git_dir: %s\n", quote_crnl(get_git_dir()));
>  	trace_printf_key(key, "setup: worktree: %s\n", quote_crnl(git_work_tree));
>  	trace_printf_key(key, "setup: cwd: %s\n", quote_crnl(cwd));
> -	trace_printf_key(key, "setup: prefix: %s\n", quote_crnl(prefix));
> +	trace_printf_key(key, "setup: prefix: %s\n", 
> +			 quote_crnl(startup_info->prefix));
>  }
>  
>  int trace_want(const char *key)

Re: [PATCH 1/3] Use startup_info->prefix rather than prefix.

[Nguyen Thai Ngoc Duy]

2012/3/3 Junio C Hamano <gitster@pobox.com>:
> Jared Hance <jaredhance@gmail.com> writes:
>
>> In trace_repo_setup, prefix is passed in as startup_info->prefix. But, as
>> indicated but a FIXME comment, trace_repo_setup has access to
>> startup_info. The prefix parameter has therefor been eliminated.
>>
>> Signed-off-by: Jared Hance <jaredhance@gmail.com>
>> ---
>
> This comes from a9ca8a8 (builtins: print setup info if repo is found,
> 2010-11-26) and hasn't ever changed over time, even across f07d6a1 (setup:
> save prefix (original cwd relative to toplevel) in startup_info,
> 2010-12-01) that did add the necessary "prefix" field to the startup_info
> and was done reasonably close to the patch that wanted to have the field
> in the first place.
>
> The fix looks too easy to be correct X-<; in other words, I find it hard
> to believe that such a triviality was left without a good reason, but I do
> not think of any.
>
> Well, but perhaps something too good to be true is indeed true sometimes.

This patch makes this function only usable when startup_info pointer
is initialized. As "git" binary is the only caller, the change is ok.
If non-builtin commands want to use this function, they need to
initialize startup_info first.
-- 
Duy

Re: [PATCH 1/3] Use startup_info->prefix rather than prefix.

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> This patch makes this function only usable when startup_info pointer
> is initialized. As "git" binary is the only caller, the change is ok.
> If non-builtin commands want to use this function, they need to
> initialize startup_info first.

Hrm, that explanation is understandable, but it strongly makes me suspect
that this change is making the code _more_ error prone in the longer term.

When somebody wants to add a new caller to a non-builtin, they need to
think about what prefix to pass, and would realize that they need to call
setup_git_directory() to get it. With the updated code, they can totally
forget and call it without any initialized startup_info.

Adding a totally new command is rare, new non-builtin is rarer, and adding
trace to it is even more so, so it may not be worth worrying about, but I
wonder if there is a cheap way to check such a programming mistake.

Re: [PATCH 1/3] Use startup_info->prefix rather than prefix.

[Jared Hance]

Junio C Hamano <gitster <at> pobox.com> writes:

> Hrm, that explanation is understandable, but it strongly makes me suspect
> that this change is making the code _more_ error prone in the longer term.
> 
> When somebody wants to add a new caller to a non-builtin, they need to
> think about what prefix to pass, and would realize that they need to call
> setup_git_directory() to get it. With the updated code, they can totally
> forget and call it without any initialized startup_info.
> 
> Adding a totally new command is rare, new non-builtin is rarer, and adding
> trace to it is even more so, so it may not be worth worrying about, but I
> wonder if there is a cheap way to check such a programming mistake.
> 

Well, there are really four options:
- Old code, where user must pass in prefix
- New code, where user must call setup_git_directory()
- Have the function deliver a nice diagnostic error message if startup_info
hasn't been initialized
- Have the function call setup_git_directory() if startup_info hasn't been
initialized

I think one of the latter two is the most sane here.

[PATCH 2/3] Fix memory leak in apply_patch in apply.c.

[Jared Hance]

In the while loop inside apply_patch, patch is dynamically allocated
with a calloc. However, only unused patches are actually free'd; the
rest are left in a memory leak. Since a list is actively built up
consisting of the used patches, they can simply be iterated and free'd
at the end of the function.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/apply.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 389898f..92ebd57 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3685,9 +3685,9 @@ static int apply_patch(int fd, const char *filename, int options)
 	size_t offset;
 	struct strbuf buf = STRBUF_INIT;
 	struct patch *list = NULL, **listp = &list;
+	struct patch *patch_iter;
 	int skipped_patch = 0;
 
-	/* FIXME - memory leak when using multiple patch files as inputs */
 	memset(&fn_table, 0, sizeof(struct string_list));
 	patch_input_file = filename;
 	read_patch_file(&buf, fd);
@@ -3712,7 +3712,6 @@ static int apply_patch(int fd, const char *filename, int options)
 			listp = &patch->next;
 		}
 		else {
-			/* perhaps free it a bit better? */
 			free(patch);
 			skipped_patch++;
 		}
@@ -3753,6 +3752,13 @@ static int apply_patch(int fd, const char *filename, int options)
 
 	if (summary)
 		summary_patch_list(list);
+	
+	patch_iter = list;
+	while(patch_iter != NULL) {
+	    struct patch *patch_iter_next = patch_iter->next;
+	    free(patch_iter);
+	    patch_iter = patch_iter_next;
+	}
 
 	strbuf_release(&buf);
 	return 0;
-- 
1.7.3.4

Re: [PATCH 2/3] Fix memory leak in apply_patch in apply.c.

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

> @@ -3712,7 +3712,6 @@ static int apply_patch(int fd, const char *filename, int options)
>  			listp = &patch->next;
>  		}
>  		else {
> -			/* perhaps free it a bit better? */
>  			free(patch);

This "free it better" comment is not about how to free the "struct patch"
itself, but is about the piece of memory pointed at it, "struct fragment",
and pieces of patch text pointed at them.  The patch text pointed with
frag->patch starts out as a location in buf.buf (which will be freed later
in this function), but IIRC there were places deeper in the callchain that
replace the pointer with allocated memory.

>  			skipped_patch++;
>  		}
> @@ -3753,6 +3752,13 @@ static int apply_patch(int fd, const char *filename, int options)
>  
>  	if (summary)
>  		summary_patch_list(list);
> +	
> +	patch_iter = list;
> +	while(patch_iter != NULL) {

	while (patch_iter) {

> +	    struct patch *patch_iter_next = patch_iter->next;
> +	    free(patch_iter);
> +	    patch_iter = patch_iter_next;
> +	}
>  
>  	strbuf_release(&buf);
>  	return 0;

[PATCH 3/3] Add threaded versions of functions in symlinks.c.

[Jared Hance]

check_leading_patch and has_dirs_only_path both always use the default
cache, which could be a caveat for adding parallelism (which is a
concern and even a GSoC proposal). This patch implements
threaded_check_leading_path and threading threaded_has_dirs_only_path
and then implements the nonthreaded functions in terms of their threaded
equivalents. No functional should be changed.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 cache.h    |    2 ++
 symlinks.c |   28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index baa8852..1113296 100644
--- a/cache.h
+++ b/cache.h
@@ -950,7 +950,9 @@ struct cache_def {
 extern int has_symlink_leading_path(const char *name, int len);
 extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
 extern int check_leading_path(const char *name, int len);
+extern int threaded_check_leading_path(struct cache_def *cache, const char *name, int len);
 extern int has_dirs_only_path(const char *name, int len, int prefix_len);
+extern int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len);
 extern void schedule_dir_for_removal(const char *name, int len);
 extern void remove_scheduled_dirs(void);
 
diff --git a/symlinks.c b/symlinks.c
index 034943b..2900367 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -219,7 +219,20 @@ int has_symlink_leading_path(const char *name, int len)
  */
 int check_leading_path(const char *name, int len)
 {
-	struct cache_def *cache = &default_cache;	/* FIXME */
+    return threaded_check_leading_path(&default_cache, name, len);
+}
+
+/*
+ * Return zero if path 'name' has a leading symlink component or
+ * if some leading path component does not exists.
+ *
+ * Return -1 if leading path exists and is a directory.
+ *
+ * Return path length if leading path exists and is neither a
+ * directory nor a symlink.
+ */
+int threaded_check_leading_path(struct cache_def *cache, const char *name, int len)
+{
 	int flags;
 	int match_len = lstat_cache_matchlen(cache, name, len, &flags,
 			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
@@ -240,7 +253,18 @@ int check_leading_path(const char *name, int len)
  */
 int has_dirs_only_path(const char *name, int len, int prefix_len)
 {
-	struct cache_def *cache = &default_cache;	/* FIXME */
+	return threaded_has_dirs_only_path(&default_cache, name, len, prefix_len);
+}
+
+/*
+ * Return non-zero if all path components of 'name' exists as a
+ * directory.  If prefix_len > 0, we will test with the stat()
+ * function instead of the lstat() function for a prefix length of
+ * 'prefix_len', thus we then allow for symlinks in the prefix part as
+ * long as those points to real existing directories.
+ */
+int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len)
+{
 	return lstat_cache(cache, name, len,
 			   FL_DIR|FL_FULLPATH, prefix_len) &
 		FL_DIR;
-- 
1.7.3.4

Re: [PATCH 3/3] Add threaded versions of functions in symlinks.c.

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

> check_leading_patch and has_dirs_only_path both always use the default

s/patch/path/;

> cache, which could be a caveat for adding parallelism (which is a
> concern and even a GSoC proposal). This patch implements
> threaded_check_leading_path and threading threaded_has_dirs_only_path
> and then implements the nonthreaded functions in terms of their threaded
> equivalents. No functional should be changed.
>
> Signed-off-by: Jared Hance <jaredhance@gmail.com>
> ---

Ok, so these FIXME's started at 867f72b (Prepare symlink caching for
thread-safety, 2009-07-09) and some of them were taken care of at b9fd284
(Export thread-safe version of 'has_symlink_leading_path()', 2009-07-09),
that follows it.  The latter left these two FIXMEs behind.

I wonder why. I suspect that it is merely because it wasn't necessary to
call these two functions from threaded context at all.

The patch changes no behaviour and should be safe, I would think.

>  cache.h    |    2 ++
>  symlinks.c |   28 ++++++++++++++++++++++++++--
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index baa8852..1113296 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -950,7 +950,9 @@ struct cache_def {
>  extern int has_symlink_leading_path(const char *name, int len);
>  extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
>  extern int check_leading_path(const char *name, int len);
> +extern int threaded_check_leading_path(struct cache_def *cache, const char *name, int len);
>  extern int has_dirs_only_path(const char *name, int len, int prefix_len);
> +extern int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len);
>  extern void schedule_dir_for_removal(const char *name, int len);
>  extern void remove_scheduled_dirs(void);
>  
> diff --git a/symlinks.c b/symlinks.c
> index 034943b..2900367 100644
> --- a/symlinks.c
> +++ b/symlinks.c
> @@ -219,7 +219,20 @@ int has_symlink_leading_path(const char *name, int len)
>   */
>  int check_leading_path(const char *name, int len)
>  {
> -	struct cache_def *cache = &default_cache;	/* FIXME */
> +    return threaded_check_leading_path(&default_cache, name, len);
> +}
> +
> +/*
> + * Return zero if path 'name' has a leading symlink component or
> + * if some leading path component does not exists.
> + *
> + * Return -1 if leading path exists and is a directory.
> + *
> + * Return path length if leading path exists and is neither a
> + * directory nor a symlink.
> + */
> +int threaded_check_leading_path(struct cache_def *cache, const char *name, int len)
> +{
>  	int flags;
>  	int match_len = lstat_cache_matchlen(cache, name, len, &flags,
>  			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
> @@ -240,7 +253,18 @@ int check_leading_path(const char *name, int len)
>   */
>  int has_dirs_only_path(const char *name, int len, int prefix_len)
>  {
> -	struct cache_def *cache = &default_cache;	/* FIXME */
> +	return threaded_has_dirs_only_path(&default_cache, name, len, prefix_len);
> +}
> +
> +/*
> + * Return non-zero if all path components of 'name' exists as a
> + * directory.  If prefix_len > 0, we will test with the stat()
> + * function instead of the lstat() function for a prefix length of
> + * 'prefix_len', thus we then allow for symlinks in the prefix part as
> + * long as those points to real existing directories.
> + */
> +int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len)
> +{
>  	return lstat_cache(cache, name, len,
>  			   FL_DIR|FL_FULLPATH, prefix_len) &
>  		FL_DIR;

[PATCH v2 0/3] Fix a few documents fixmes

[Jared Hance]

A few patches that (hopefully) don't change the behavior of git except to
rectify a memory error. Also, this should ever so slightly help with
the parallelism of git (a GSoC proposal). All of these were found with
commented FIXME and git grep.

Jared Hance (3):
  Use startup_info->prefix rather than prefix.
  Fix memory leak in apply_patch in apply.c.
  Add threaded versions of functions in symlinks.c.

 builtin/apply.c |   30 +++++++++++++++++++++++++++---
 cache.h         |    4 +++-
 git.c           |    2 +-
 symlinks.c      |   28 ++++++++++++++++++++++++++--
 trace.c         |   10 +++++-----
 5 files changed, 62 insertions(+), 12 deletions(-)

-- 

Changelog since last series submission:
    - Whitespace fixes that I should have checked for before
    - Typo fix
    - Completely redo my fix to the memory leak to address issues pointed
      out by Junio.

Note: I'm not quite sure if I actually agree with the first change. It makes
sense right now if git.c is the only caller, but in the future, it might become
less flexible. It might be wise to actually make startup_info static to git.c
for protection in the future. They should be relatively independent, though,
so the 2nd and 3rd which are probably less controversal can easily be applied.

1.7.3.4

[PATCH v2 1/3] Use startup_info->prefix rather than prefix.

[Jared Hance]

In trace_repo_setup, prefix is passed in as startup_info->prefix. But, as
indicated but a FIXME comment, trace_repo_setup has access to
startup_info. The prefix parameter has therefor been eliminated.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 cache.h |    2 +-
 git.c   |    2 +-
 trace.c |   10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index e12b15f..baa8852 100644
--- a/cache.h
+++ b/cache.h
@@ -1211,7 +1211,7 @@ extern void trace_printf(const char *format, ...);
 extern void trace_vprintf(const char *key, const char *format, va_list ap);
 __attribute__((format (printf, 2, 3)))
 extern void trace_argv_printf(const char **argv, const char *format, ...);
-extern void trace_repo_setup(const char *prefix);
+extern void trace_repo_setup(void);
 extern int trace_want(const char *key);
 extern void trace_strbuf(const char *key, const struct strbuf *buf);
 
diff --git a/git.c b/git.c
index 3805616..7dcc527 100644
--- a/git.c
+++ b/git.c
@@ -296,7 +296,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
 		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
-			trace_repo_setup(prefix);
+			trace_repo_setup();
 	}
 	commit_pager_choice();
 
diff --git a/trace.c b/trace.c
index d953416..09a470b 100644
--- a/trace.c
+++ b/trace.c
@@ -152,8 +152,7 @@ static const char *quote_crnl(const char *path)
 	return new_path;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
+void trace_repo_setup(void)
 {
 	static const char *key = "GIT_TRACE_SETUP";
 	const char *git_work_tree;
@@ -168,13 +167,14 @@ void trace_repo_setup(const char *prefix)
 	if (!(git_work_tree = get_git_work_tree()))
 		git_work_tree = "(null)";
 
-	if (!prefix)
-		prefix = "(null)";
+	if (!startup_info->prefix)
+		startup_info->prefix = "(null)";
 
 	trace_printf_key(key, "setup: git_dir: %s\n", quote_crnl(get_git_dir()));
 	trace_printf_key(key, "setup: worktree: %s\n", quote_crnl(git_work_tree));
 	trace_printf_key(key, "setup: cwd: %s\n", quote_crnl(cwd));
-	trace_printf_key(key, "setup: prefix: %s\n", quote_crnl(prefix));
+	trace_printf_key(key, "setup: prefix: %s\n",
+			 quote_crnl(startup_info->prefix));
 }
 
 int trace_want(const char *key)
-- 
1.7.3.4

Re: [PATCH v2 1/3] Use startup_info->prefix rather than prefix.

[Jeff Epler]

On Sat, Mar 03, 2012 at 09:40:28AM -0500, Jared Hance wrote:
> In trace_repo_setup, prefix is passed in as startup_info->prefix. But, as
> indicated but a FIXME comment, trace_repo_setup has access to
> startup_info. The prefix parameter has therefor been eliminated.
                                         ^^^^^^^^
Trivial typo, should be "therefore".

Jeff

[PATCH v2 2/3] Fix memory leak in apply_patch in apply.c.

[Jared Hance]

In the while loop inside apply_patch, patch is dynamically allocated
with a calloc. However, only unused patches are actually free'd; the
rest are left in a memory leak. Since a list is actively built up
consisting of the used patches, they can simply be iterated and free'd
at the end of the function.

In addition, the list of fragments should be free'd. To fix this, the
utility function free_patch has been implemented. It loops over the
entire patch list, and in each patch, loops over the fragment list,
freeing the fragments, followed by the patch in the list. It frees both
patch and patch->next.

The main caveat is that the text in a fragment, ie,
patch->fragments->patch, may or may not need to be free'd. The text is
dynamically allocated and needs to be freed iff the patch is a binary
patch, as allocation occurs in inflate_it.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/apply.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 389898f..a73d339 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -196,6 +196,30 @@ struct patch {
 	struct patch *next;
 };
 
+static void free_patch(struct patch *patch) {
+    while(patch != NULL) {
+	struct patch *patch_next;
+	struct fragment *fragment;
+
+	patch_next = patch->next;
+
+	fragment = patch->fragments;
+	while(fragment != NULL) {
+	    struct fragment *fragment_next = fragment->next;
+	    if(fragment->patch != NULL) {
+		if(patch->is_binary) {
+		    free((void*) fragment->patch);
+		}
+	    }
+	    free(fragment);
+	    fragment = fragment_next;
+	}
+
+	free(patch);
+	patch = patch_next;
+    }
+}
+
 /*
  * A line in a file, len-bytes long (includes the terminating LF,
  * except for an incomplete line at the end if the file ends with
@@ -3687,7 +3711,6 @@ static int apply_patch(int fd, const char *filename, int options)
 	struct patch *list = NULL, **listp = &list;
 	int skipped_patch = 0;
 
-	/* FIXME - memory leak when using multiple patch files as inputs */
 	memset(&fn_table, 0, sizeof(struct string_list));
 	patch_input_file = filename;
 	read_patch_file(&buf, fd);
@@ -3712,8 +3735,7 @@ static int apply_patch(int fd, const char *filename, int options)
 			listp = &patch->next;
 		}
 		else {
-			/* perhaps free it a bit better? */
-			free(patch);
+			free_patch(patch);
 			skipped_patch++;
 		}
 		offset += nr;
@@ -3754,6 +3776,8 @@ static int apply_patch(int fd, const char *filename, int options)
 	if (summary)
 		summary_patch_list(list);
 
+	free_patch(list);
+
 	strbuf_release(&buf);
 	return 0;
 }
-- 
1.7.3.4

Re: [PATCH v2 2/3] Fix memory leak in apply_patch in apply.c.

[Jared Hance]

I just realized I'll probably need to resend this one due to stylistic
errors (not using space with loops and if...).

Hopefully if there are no other mistakes I can just send in a purely
cosmetically changed version to Junio.

Re: [PATCH v2 2/3] Fix memory leak in apply_patch in apply.c.

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

> In addition, the list of fragments should be free'd. To fix this, the
> utility function free_patch has been implemented. It loops over the
> entire patch list, and in each patch, loops over the fragment list,
> freeing the fragments, followed by the patch in the list. It frees both
> patch and patch->next.

Right encapsulation and abstraction. Good.

>
> The main caveat is that the text in a fragment, ie,
> patch->fragments->patch, may or may not need to be free'd. The text is
> dynamically allocated and needs to be freed iff the patch is a binary
> patch, as allocation occurs in inflate_it.

Can't we do better than "is this for a binary patch"?  I find this part
not very forward-thinking implementation that relies on an implementation
detail that happens to hold true for today's code.

At least

        if ((buf.buf <= fragment->patch &&
            (fragment->patch < buf.buf + buf.len))
		; /* this is inside the original buffer */
	else
		free(fragment->patch);

or something?

Strictly speaking, even the above is not forward-thinking enough, as the
code deep in the callchain is free to replace ->patch with an unfreeable
string.  I think the right way to handle this is to add a single bitfield
"should_free" to the struct fragment and default it to 'false', and make
the place that replace the patch field with different string responsible
for flipping the bit to 'true'.  Your "free_patch()" can then rely on that
bit to make the decision.

> Signed-off-by: Jared Hance <jaredhance@gmail.com>
> ---
>  builtin/apply.c |   30 +++++++++++++++++++++++++++---
>  1 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 389898f..a73d339 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -196,6 +196,30 @@ struct patch {
>  	struct patch *next;
>  };
>  
> +static void free_patch(struct patch *patch) {
> +    while(patch != NULL) {

Style:

	static void free_patch(struct patch *patch)
	{
		while (patch) {
			...

Thanks.

[PATCH v2 3/3] Add threaded versions of functions in symlinks.c.

[Jared Hance]

check_leading_path and has_dirs_only_path both always use the default
cache, which could be a caveat for adding parallelism (which is a
concern and even a GSoC proposal). This patch implements
threaded_check_leading_path and threading threaded_has_dirs_only_path
and then implements the nonthreaded functions in terms of their threaded
equivalents. No functional should be changed.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 cache.h    |    2 ++
 symlinks.c |   28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index baa8852..1113296 100644
--- a/cache.h
+++ b/cache.h
@@ -950,7 +950,9 @@ struct cache_def {
 extern int has_symlink_leading_path(const char *name, int len);
 extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
 extern int check_leading_path(const char *name, int len);
+extern int threaded_check_leading_path(struct cache_def *cache, const char *name, int len);
 extern int has_dirs_only_path(const char *name, int len, int prefix_len);
+extern int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len);
 extern void schedule_dir_for_removal(const char *name, int len);
 extern void remove_scheduled_dirs(void);
 
diff --git a/symlinks.c b/symlinks.c
index 034943b..2900367 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -219,7 +219,20 @@ int has_symlink_leading_path(const char *name, int len)
  */
 int check_leading_path(const char *name, int len)
 {
-	struct cache_def *cache = &default_cache;	/* FIXME */
+    return threaded_check_leading_path(&default_cache, name, len);
+}
+
+/*
+ * Return zero if path 'name' has a leading symlink component or
+ * if some leading path component does not exists.
+ *
+ * Return -1 if leading path exists and is a directory.
+ *
+ * Return path length if leading path exists and is neither a
+ * directory nor a symlink.
+ */
+int threaded_check_leading_path(struct cache_def *cache, const char *name, int len)
+{
 	int flags;
 	int match_len = lstat_cache_matchlen(cache, name, len, &flags,
 			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
@@ -240,7 +253,18 @@ int check_leading_path(const char *name, int len)
  */
 int has_dirs_only_path(const char *name, int len, int prefix_len)
 {
-	struct cache_def *cache = &default_cache;	/* FIXME */
+	return threaded_has_dirs_only_path(&default_cache, name, len, prefix_len);
+}
+
+/*
+ * Return non-zero if all path components of 'name' exists as a
+ * directory.  If prefix_len > 0, we will test with the stat()
+ * function instead of the lstat() function for a prefix length of
+ * 'prefix_len', thus we then allow for symlinks in the prefix part as
+ * long as those points to real existing directories.
+ */
+int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len)
+{
 	return lstat_cache(cache, name, len,
 			   FL_DIR|FL_FULLPATH, prefix_len) &
 		FL_DIR;
-- 
1.7.3.4

Re: [PATCH v2 3/3] Add threaded versions of functions in symlinks.c.

[Thomas Rast]

Jared Hance <jaredhance@gmail.com> writes:

>  int check_leading_path(const char *name, int len)
>  {
> -	struct cache_def *cache = &default_cache;	/* FIXME */
> +    return threaded_check_leading_path(&default_cache, name, len);
> +}

This hunk is indent-by-space damaged.

Otherwise the patch looks good to me.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch