threads / patch / 42512

patchpathspec: rename free_pathspec() to clear_pathspec()

Subject: [PATCH] pathspec: rename free_pathspec() to clear_pathspec()

## tl;dr

3 messages between Jun 2, 2016 and Jun 2, 2016. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Junio C Hamano· Jun 2, 2016, 21:18 UTC · lore

The function takes a pointer to a pathspec structure, and releases the resources held by it, but does not free() the structure itself. Such a function should be called "clear", not "free".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This is just something I noticed.  Among the hits in
    $ git grep free_ \*.h
   I think free_notes() is also a candidate for such renaming, but
   because we are not actively working on that subsystem, we may
   want to leave that dog sleeping to avoid unnecessary code churn.
   The same for diff_free_filespec_data(), for which a better name
   would have been diff_filespec_clear().
 archive.c              | 2 +-
 builtin/blame.c        | 6 +++---
 builtin/reset.c        | 2 +-
 builtin/update-index.c | 2 +-
 combine-diff.c         | 2 +-
 notes-merge.c          | 4 ++--
 pathspec.c             | 2 +-
 pathspec.h             | 4 ++--
 revision.c             | 2 +-
 tree-diff.c            | 4 ++--
 10 files changed, 15 insertions(+), 15 deletions(-)
Show changes to 10 files +15 −15

archive.c, builtin/blame.c, builtin/reset.c, builtin/update-index.c, combine-diff.c, notes-merge.c, pathspec.c, pathspec.h, revision.c, tree-diff.c

diff --git a/archive.c b/archive.c
index 5d735ae..42df974 100644
--- a/archive.c
+++ b/archive.c
@@ -322,7 +322,7 @@ static int path_exists(struct tree *tree, const char *path)
 	pathspec.recursive = 1;
 	ret = read_tree_recursive(tree, "", 0, 0, &pathspec,
 				  reject_entry, &pathspec);
-	free_pathspec(&pathspec);
+	clear_pathspec(&pathspec);
 	return ret != 0;
 }
 
diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0..759d84a 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -609,7 +609,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 		}
 	}
 	diff_flush(&diff_opts);
-	free_pathspec(&diff_opts.pathspec);
+	clear_pathspec(&diff_opts.pathspec);
 	return porigin;
 }
 
@@ -651,7 +651,7 @@ static struct origin *find_rename(struct scoreboard *sb,
 		}
 	}
 	diff_flush(&diff_opts);
-	free_pathspec(&diff_opts.pathspec);
+	clear_pathspec(&diff_opts.pathspec);
 	return porigin;
 }
 
@@ -1343,7 +1343,7 @@ static void find_copy_in_parent(struct scoreboard *sb,
 	} while (unblamed);
 	target->suspects = reverse_blame(leftover, NULL);
 	diff_flush(&diff_opts);
-	free_pathspec(&diff_opts.pathspec);
+	clear_pathspec(&diff_opts.pathspec);
 }
 
 /*
diff --git a/builtin/reset.c b/builtin/reset.c
index 092c3a5..acd6278 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -158,7 +158,7 @@ static int read_from_tree(const struct pathspec *pathspec,
 		return 1;
 	diffcore_std(&opt);
 	diff_flush(&opt);
-	free_pathspec(&opt.pathspec);
+	clear_pathspec(&opt.pathspec);
 
 	return 0;
 }
diff --git a/builtin/update-index.c b/builtin/update-index.c
index b8b8522..6cdfd5f 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -759,7 +759,7 @@ static int do_reupdate(int ac, const char **av,
 		if (save_nr != active_nr)
 			goto redo;
 	}
-	free_pathspec(&pathspec);
+	clear_pathspec(&pathspec);
 	return 0;
 }
 
diff --git a/combine-diff.c b/combine-diff.c
index 8f2313d..5920df8 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1525,7 +1525,7 @@ void diff_tree_combined(const unsigned char *sha1,
 		free(tmp);
 	}
 
-	free_pathspec(&diffopts.pathspec);
+	clear_pathspec(&diffopts.pathspec);
 }
 
 void diff_tree_combined_merge(const struct commit *commit, int dense,
diff --git a/notes-merge.c b/notes-merge.c
index 34bfac0..b7814c9 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -170,7 +170,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
 		       sha1_to_hex(mp->remote));
 	}
 	diff_flush(&opt);
-	free_pathspec(&opt.pathspec);
+	clear_pathspec(&opt.pathspec);
 
 	*num_changes = len;
 	return changes;
@@ -256,7 +256,7 @@ static void diff_tree_local(struct notes_merge_options *o,
 		       sha1_to_hex(mp->local));
 	}
 	diff_flush(&opt);
-	free_pathspec(&opt.pathspec);
+	clear_pathspec(&opt.pathspec);
 }
 
 static void check_notes_merge_worktree(struct notes_merge_options *o)
diff --git a/pathspec.c b/pathspec.c
index c9e9b6c..24e0dd5 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -489,7 +489,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)
 	       sizeof(struct pathspec_item) * dst->nr);
 }
 
-void free_pathspec(struct pathspec *pathspec)
+void clear_pathspec(struct pathspec *pathspec)
 {
 	free(pathspec->items);
 	pathspec->items = NULL;
diff --git a/pathspec.h b/pathspec.h
index 0c11262..4a80f6f 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -19,7 +19,7 @@
 #define PATHSPEC_ONESTAR 1	/* the pathspec pattern satisfies GFNM_ONESTAR */
 
 struct pathspec {
-	const char **_raw; /* get_pathspec() result, not freed by free_pathspec() */
+	const char **_raw; /* get_pathspec() result, not freed by clear_pathspec() */
 	int nr;
 	unsigned int has_wildcard:1;
 	unsigned int recursive:1;
@@ -74,7 +74,7 @@ extern void parse_pathspec(struct pathspec *pathspec,
 			   const char *prefix,
 			   const char **args);
 extern void copy_pathspec(struct pathspec *dst, const struct pathspec *src);
-extern void free_pathspec(struct pathspec *);
+extern void clear_pathspec(struct pathspec *);
 
 static inline int ps_strncmp(const struct pathspec_item *item,
 			     const char *s1, const char *s2, size_t n)
diff --git a/revision.c b/revision.c
index d30d1c4..2f60062 100644
--- a/revision.c
+++ b/revision.c
@@ -1425,7 +1425,7 @@ static void prepare_show_merge(struct rev_info *revs)
 		       ce_same_name(ce, active_cache[i+1]))
 			i++;
 	}
-	free_pathspec(&revs->prune_data);
+	clear_pathspec(&revs->prune_data);
 	parse_pathspec(&revs->prune_data, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
 		       PATHSPEC_PREFER_FULL | PATHSPEC_LITERAL_PATH, "", prune);
 	revs->limited = 1;
diff --git a/tree-diff.c b/tree-diff.c
index ff4e0d3..edb4de9 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -607,7 +607,7 @@ static void try_to_follow_renames(const unsigned char *old, const unsigned char
 	diff_setup_done(&diff_opts);
 	ll_diff_tree_sha1(old, new, base, &diff_opts);
 	diffcore_std(&diff_opts);
-	free_pathspec(&diff_opts.pathspec);
+	clear_pathspec(&diff_opts.pathspec);
 
 	/* Go through the new set of filepairing, and see if we find a more interesting one */
 	opt->found_follow = 0;
@@ -630,7 +630,7 @@ static void try_to_follow_renames(const unsigned char *old, const unsigned char
 			/* Update the path we use from now on.. */
 			path[0] = p->one->path;
 			path[1] = NULL;
-			free_pathspec(&opt->pathspec);
+			clear_pathspec(&opt->pathspec);
 			parse_pathspec(&opt->pathspec,
 				       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,
 				       PATHSPEC_LITERAL_PATH, "", path);

-- 
2.9.0-rc1-228-gd00d833
Jeff King· Jun 2, 2016, 21:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] pathspec: rename free_pathspec() to clear_pathspec()

On Thu, Jun 02, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:
> The function takes a pointer to a pathspec structure, and releases
> the resources held by it, but does not free() the structure itself.
> Such a function should be called "clear", not "free".
Hmm, makes sense, though...
Show 9 quoted lines
>  * This is just something I noticed.  Among the hits in
> 
>     $ git grep free_ \*.h
> 
>    I think free_notes() is also a candidate for such renaming, but
>    because we are not actively working on that subsystem, we may
>    want to leave that dog sleeping to avoid unnecessary code churn.
>    The same for diff_free_filespec_data(), for which a better name
>    would have been diff_filespec_clear().

I think diff_filespec_clear() would not be quite right. It is freeing only the allocated _data_, but leaving the other portions intact. Generally our _clear() functions reset the object back to an initial state, from which it can be reused. I don't see that as a big problem because there is an other object for the verb "free" here: "data". We are just freeing its data, but the rest of the object remains intact and we may fill in the data again later.

But I think pathspec is in similar boat; it has not been cleared back to its initial state. But it is in a much _worse_ state than the filespec, which you can continue to use. It is in a totally broken state where "nr" does not correspond to the actual number of items, the has_wildcard flag is bogus, etc.

So I think it would be OK to move it to "clear", but we should probably also zero the whole thing, too.

-Peff
Junio C Hamano· Jun 2, 2016, 22:03 UTC · re: Jeff King · lore

Re: [PATCH] pathspec: rename free_pathspec() to clear_pathspec()

Jeff King <peff@peff.net> writes:
> I think diff_filespec_clear() would not be quite right. It is freeing
> only the allocated _data_, but leaving the other portions intact.
You are (as usual) more right than I am ;-)

Yes, the free_data() is designed to retain enough information about the filespec so that the data can be re-read by the next person who needs to access it after free_data() is called; i.e. it was a measure to reduce the memory pressure by trading it off with I/O cost. It is wrong to name it "clear", which is to "clear the slate, make it into pristine state" whose side effect is to release held resources.

← back to recent threads