[PATCH v3 0/3] Fix documented fixme's throughout

[PATCH v3 0/3] Fix documented fixme's throughout

[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):
  Fix memory leak in apply_patch in apply.c.
  Add threaded versions of functions in symlinks.c.
  Use startup_info->prefix rather than prefix.

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

-- 

Minor style changes and move the most controversial commit to the end of the
patch series.

Sorry, accidently sent out an old saved version of this just a bit before.

1.7.3.4

[PATCH v3 1/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 |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 389898f..4c6b278 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -153,6 +153,7 @@ struct fragment {
 	unsigned long oldpos, oldlines;
 	unsigned long newpos, newlines;
 	const char *patch;
+	unsigned int free_patch:1;
 	int size;
 	int rejected;
 	int linenr;
@@ -196,6 +197,29 @@ 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 && fragment->free_patch) {
+				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
@@ -1742,6 +1766,7 @@ static struct fragment *parse_binary_hunk(char **buf_p,
 
 	frag = xcalloc(1, sizeof(*frag));
 	frag->patch = inflate_it(data, hunk_size, origlen);
+	frag->free_patch = 1;
 	if (!frag->patch)
 		goto corrupt;
 	free(data);
@@ -3687,7 +3712,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 +3736,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 +3777,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 v3 1/3] Fix memory leak in apply_patch in apply.c.

[Junio C Hamano]

Jared Hance <jaredhance@gmail.com> writes:

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

Thanks.

This more-or-less looks good modulo minor style issues.  We might
also want to make rejected a one-bit bitfield that sits next to the
new free_patch field to share the same word, but that is a separate
topic.

Will queue.

[PATCH 1/2] apply: free patch->{def,old,new}_name fields

[Junio C Hamano]

These were all allocated in the heap by parsing the header parts of the
patch, but we did not bother to free them.  Some used to share the memory
(e.g. copying def_name to old_name) so this is not just the matter of
adding three calls to free().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c |   65 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 427c263..5d03e50 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -209,6 +209,9 @@ static void free_patch(struct patch *patch)
 			free(fragment);
 			fragment = fragment_next;
 		}
+		free(patch->def_name);
+		free(patch->old_name);
+		free(patch->new_name);
 		free(patch);
 		patch = patch_next;
 	}
@@ -434,7 +437,7 @@ static char *squash_slash(char *name)
 	return name;
 }
 
-static char *find_name_gnu(const char *line, char *def, int p_value)
+static char *find_name_gnu(const char *line, const char *def, int p_value)
 {
 	struct strbuf name = STRBUF_INIT;
 	char *cp;
@@ -457,11 +460,7 @@ static char *find_name_gnu(const char *line, char *def, int p_value)
 		cp++;
 	}
 
-	/* name can later be freed, so we need
-	 * to memmove, not just return cp
-	 */
 	strbuf_remove(&name, 0, cp - name.buf);
-	free(def);
 	if (root)
 		strbuf_insert(&name, 0, root, root_len);
 	return squash_slash(strbuf_detach(&name, NULL));
@@ -626,8 +625,13 @@ static size_t diff_timestamp_len(const char *line, size_t len)
 	return line + len - end;
 }
 
-static char *find_name_common(const char *line, char *def, int p_value,
-				const char *end, int terminate)
+static char *null_strdup(const char *s)
+{
+	return s ? xstrdup(s) : NULL;
+}
+
+static char *find_name_common(const char *line, const char *def,
+			      int p_value, const char *end, int terminate)
 {
 	int len;
 	const char *start = NULL;
@@ -648,10 +652,10 @@ static char *find_name_common(const char *line, char *def, int p_value,
 			start = line;
 	}
 	if (!start)
-		return squash_slash(def);
+		return squash_slash(null_strdup(def));
 	len = line - start;
 	if (!len)
-		return squash_slash(def);
+		return squash_slash(null_strdup(def));
 
 	/*
 	 * Generally we prefer the shorter name, especially
@@ -662,8 +666,7 @@ static char *find_name_common(const char *line, char *def, int p_value,
 	if (def) {
 		int deflen = strlen(def);
 		if (deflen < len && !strncmp(start, def, deflen))
-			return squash_slash(def);
-		free(def);
+			return squash_slash(xstrdup(def));
 	}
 
 	if (root) {
@@ -860,8 +863,10 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 		name = find_name_traditional(first, NULL, p_value);
 		patch->old_name = name;
 	} else {
-		name = find_name_traditional(first, NULL, p_value);
-		name = find_name_traditional(second, name, p_value);
+		char *first_name;
+		first_name = find_name_traditional(first, NULL, p_value);
+		name = find_name_traditional(second, first_name, p_value);
+		free(first_name);
 		if (has_epoch_timestamp(first)) {
 			patch->is_new = 1;
 			patch->is_delete = 0;
@@ -871,7 +876,8 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 			patch->is_delete = 1;
 			patch->old_name = name;
 		} else {
-			patch->old_name = patch->new_name = name;
+			patch->old_name = name;
+			patch->new_name = xstrdup(name);
 		}
 	}
 	if (!name)
@@ -921,13 +927,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,
 
 static int gitdiff_oldname(const char *line, struct patch *patch)
 {
+	char *orig = patch->old_name;
 	patch->old_name = gitdiff_verify_name(line, patch->is_new, patch->old_name, "old");
+	if (orig != patch->old_name)
+		free(orig);
 	return 0;
 }
 
 static int gitdiff_newname(const char *line, struct patch *patch)
 {
+	char *orig = patch->new_name;
 	patch->new_name = gitdiff_verify_name(line, patch->is_delete, patch->new_name, "new");
+	if (orig != patch->new_name)
+		free(orig);
 	return 0;
 }
 
@@ -946,20 +958,23 @@ static int gitdiff_newmode(const char *line, struct patch *patch)
 static int gitdiff_delete(const char *line, struct patch *patch)
 {
 	patch->is_delete = 1;
-	patch->old_name = patch->def_name;
+	free(patch->old_name);
+	patch->old_name = null_strdup(patch->def_name);
 	return gitdiff_oldmode(line, patch);
 }
 
 static int gitdiff_newfile(const char *line, struct patch *patch)
 {
 	patch->is_new = 1;
-	patch->new_name = patch->def_name;
+	free(patch->new_name);
+	patch->new_name = null_strdup(patch->def_name);
 	return gitdiff_newmode(line, patch);
 }
 
 static int gitdiff_copysrc(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
+	free(patch->old_name);
 	patch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -967,6 +982,7 @@ static int gitdiff_copysrc(const char *line, struct patch *patch)
 static int gitdiff_copydst(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
+	free(patch->new_name);
 	patch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -974,6 +990,7 @@ static int gitdiff_copydst(const char *line, struct patch *patch)
 static int gitdiff_renamesrc(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
+	free(patch->old_name);
 	patch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -981,6 +998,7 @@ static int gitdiff_renamesrc(const char *line, struct patch *patch)
 static int gitdiff_renamedst(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
+	free(patch->new_name);
 	patch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -1421,7 +1439,8 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 				if (!patch->def_name)
 					die("git diff header lacks filename information when removing "
 					    "%d leading pathname components (line %d)" , p_value, linenr);
-				patch->old_name = patch->new_name = patch->def_name;
+				patch->old_name = xstrdup(patch->def_name);
+				patch->new_name = xstrdup(patch->def_name);
 			}
 			if (!patch->is_delete && !patch->new_name)
 				die("git diff header lacks filename information "
@@ -3104,6 +3123,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
  is_new:
 	patch->is_new = 1;
 	patch->is_delete = 0;
+	free(patch->old_name);
 	patch->old_name = NULL;
 	return 0;
 }
@@ -3684,15 +3704,8 @@ static void prefix_patches(struct patch *p)
 	if (!prefix || p->is_toplevel_relative)
 		return;
 	for ( ; p; p = p->next) {
-		if (p->new_name == p->old_name) {
-			char *prefixed = p->new_name;
-			prefix_one(&prefixed);
-			p->new_name = p->old_name = prefixed;
-		}
-		else {
-			prefix_one(&p->new_name);
-			prefix_one(&p->old_name);
-		}
+		prefix_one(&p->new_name);
+		prefix_one(&p->old_name);
 	}
 }
 
-- 
1.7.10.rc1.76.g1a8310

[PATCH 2/2] apply: free patch->result

[Junio C Hamano]

This is by far the largest piece of data, much larger than the patch and
fragment structures or the three name fields in the patch structure.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
I have not finished auditing all the codepaths, so this change needs to be
eyeballed carefully. We may be pointing an unfreeable piece of memory or a
piece of memory that belong to other structures that are going to be freed
otherwise.

 builtin/apply.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index 5d03e50..c919db3 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -212,6 +212,7 @@ static void free_patch(struct patch *patch)
 		free(patch->def_name);
 		free(patch->old_name);
 		free(patch->new_name);
+		free(patch->result);
 		free(patch);
 		patch = patch_next;
 	}
-- 
1.7.10.rc1.76.g1a8310

[PATCH v3 2/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 e12b15f..e5e1aa4 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 v3 2/3] Add threaded versions of functions in symlinks.c.

[Junio C Hamano]

This is identical to what is listed as "Cooking already in 'next'"
in the latest issue of "What's cooking", and I've already merged it
to 'master' in preparation for 1.7.10-rc0.

Thanks.

[PATCH v3 3/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 therefore 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 e5e1aa4..1113296 100644
--- a/cache.h
+++ b/cache.h
@@ -1213,7 +1213,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 v3 3/3] Use startup_info->prefix rather than prefix.

[Junio C Hamano]

Nothing seems to have changed in this patch since the last round.

My impression has been that we agreed with your comment

    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.

and decided to drop this patch at least for now.