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

DORMANTno replies

7 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Jared Hance <hidden>
Date: 2016-06-15 22:53:14

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.

From: Jared Hance <hidden>
Date: 2016-06-15 22:53:14

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

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

From: Jared Hance <hidden>
Date: 2016-06-15 22:53:14

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

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

From: Jared Hance <hidden>
Date: 2016-06-15 22:53:14

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 <redacted>
---
 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 2/3] Add threaded versions of functions in symlinks.c.

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:14

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.

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:14

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.

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:14

Jared Hance [off-list ref] 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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help