{"thread":{"id":"42512","subject":"[PATCH] pathspec: rename free_pathspec() to clear_pathspec()","startedAt":"2016-06-02T21:18:47Z","lastAt":"2016-06-02T22:03:15Z","messageCount":3,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"288157","messageId":"xmqq60trthl4.fsf@gitster.mtv.corp.google.com","threadId":"42512","inReplyTo":null,"subject":"[PATCH] pathspec: rename free_pathspec() to clear_pathspec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T21:18:47Z","receivedAt":"2016-06-02T21:18:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The function takes a pointer to a pathspec structure, and releases\nthe resources held by it, but does not free() the structure itself.\nSuch a function should be called \"clear\", not \"free\".\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is just something I noticed.  Among the hits in\n\n    $ git grep free_ \\*.h\n\n   I think free_notes() is also a candidate for such renaming, but\n   because we are not actively working on that subsystem, we may\n   want to leave that dog sleeping to avoid unnecessary code churn.\n   The same for diff_free_filespec_data(), for which a better name\n   would have been diff_filespec_clear().\n\n archive.c              | 2 +-\n builtin/blame.c        | 6 +++---\n builtin/reset.c        | 2 +-\n builtin/update-index.c | 2 +-\n combine-diff.c         | 2 +-\n notes-merge.c          | 4 ++--\n pathspec.c             | 2 +-\n pathspec.h             | 4 ++--\n revision.c             | 2 +-\n tree-diff.c            | 4 ++--\n 10 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 5d735ae..42df974 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -322,7 +322,7 @@ static int path_exists(struct tree *tree, const char *path)\n \tpathspec.recursive = 1;\n \tret = read_tree_recursive(tree, \"\", 0, 0, &pathspec,\n \t\t\t\t  reject_entry, &pathspec);\n-\tfree_pathspec(&pathspec);\n+\tclear_pathspec(&pathspec);\n \treturn ret != 0;\n }\n \ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 21f42b0..759d84a 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -609,7 +609,7 @@ static struct origin *find_origin(struct scoreboard *sb,\n \t\t}\n \t}\n \tdiff_flush(&diff_opts);\n-\tfree_pathspec(&diff_opts.pathspec);\n+\tclear_pathspec(&diff_opts.pathspec);\n \treturn porigin;\n }\n \n@@ -651,7 +651,7 @@ static struct origin *find_rename(struct scoreboard *sb,\n \t\t}\n \t}\n \tdiff_flush(&diff_opts);\n-\tfree_pathspec(&diff_opts.pathspec);\n+\tclear_pathspec(&diff_opts.pathspec);\n \treturn porigin;\n }\n \n@@ -1343,7 +1343,7 @@ static void find_copy_in_parent(struct scoreboard *sb,\n \t} while (unblamed);\n \ttarget->suspects = reverse_blame(leftover, NULL);\n \tdiff_flush(&diff_opts);\n-\tfree_pathspec(&diff_opts.pathspec);\n+\tclear_pathspec(&diff_opts.pathspec);\n }\n \n /*\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 092c3a5..acd6278 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -158,7 +158,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \t\treturn 1;\n \tdiffcore_std(&opt);\n \tdiff_flush(&opt);\n-\tfree_pathspec(&opt.pathspec);\n+\tclear_pathspec(&opt.pathspec);\n \n \treturn 0;\n }\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex b8b8522..6cdfd5f 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -759,7 +759,7 @@ static int do_reupdate(int ac, const char **av,\n \t\tif (save_nr != active_nr)\n \t\t\tgoto redo;\n \t}\n-\tfree_pathspec(&pathspec);\n+\tclear_pathspec(&pathspec);\n \treturn 0;\n }\n \ndiff --git a/combine-diff.c b/combine-diff.c\nindex 8f2313d..5920df8 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1525,7 +1525,7 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\tfree(tmp);\n \t}\n \n-\tfree_pathspec(&diffopts.pathspec);\n+\tclear_pathspec(&diffopts.pathspec);\n }\n \n void diff_tree_combined_merge(const struct commit *commit, int dense,\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 34bfac0..b7814c9 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -170,7 +170,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,\n \t\t       sha1_to_hex(mp->remote));\n \t}\n \tdiff_flush(&opt);\n-\tfree_pathspec(&opt.pathspec);\n+\tclear_pathspec(&opt.pathspec);\n \n \t*num_changes = len;\n \treturn changes;\n@@ -256,7 +256,7 @@ static void diff_tree_local(struct notes_merge_options *o,\n \t\t       sha1_to_hex(mp->local));\n \t}\n \tdiff_flush(&opt);\n-\tfree_pathspec(&opt.pathspec);\n+\tclear_pathspec(&opt.pathspec);\n }\n \n static void check_notes_merge_worktree(struct notes_merge_options *o)\ndiff --git a/pathspec.c b/pathspec.c\nindex c9e9b6c..24e0dd5 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -489,7 +489,7 @@ void copy_pathspec(struct pathspec *dst, const struct pathspec *src)\n \t       sizeof(struct pathspec_item) * dst->nr);\n }\n \n-void free_pathspec(struct pathspec *pathspec)\n+void clear_pathspec(struct pathspec *pathspec)\n {\n \tfree(pathspec->items);\n \tpathspec->items = NULL;\ndiff --git a/pathspec.h b/pathspec.h\nindex 0c11262..4a80f6f 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -19,7 +19,7 @@\n #define PATHSPEC_ONESTAR 1\t/* the pathspec pattern satisfies GFNM_ONESTAR */\n \n struct pathspec {\n-\tconst char **_raw; /* get_pathspec() result, not freed by free_pathspec() */\n+\tconst char **_raw; /* get_pathspec() result, not freed by clear_pathspec() */\n \tint nr;\n \tunsigned int has_wildcard:1;\n \tunsigned int recursive:1;\n@@ -74,7 +74,7 @@ extern void parse_pathspec(struct pathspec *pathspec,\n \t\t\t   const char *prefix,\n \t\t\t   const char **args);\n extern void copy_pathspec(struct pathspec *dst, const struct pathspec *src);\n-extern void free_pathspec(struct pathspec *);\n+extern void clear_pathspec(struct pathspec *);\n \n static inline int ps_strncmp(const struct pathspec_item *item,\n \t\t\t     const char *s1, const char *s2, size_t n)\ndiff --git a/revision.c b/revision.c\nindex d30d1c4..2f60062 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1425,7 +1425,7 @@ static void prepare_show_merge(struct rev_info *revs)\n \t\t       ce_same_name(ce, active_cache[i+1]))\n \t\t\ti++;\n \t}\n-\tfree_pathspec(&revs->prune_data);\n+\tclear_pathspec(&revs->prune_data);\n \tparse_pathspec(&revs->prune_data, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,\n \t\t       PATHSPEC_PREFER_FULL | PATHSPEC_LITERAL_PATH, \"\", prune);\n \trevs->limited = 1;\ndiff --git a/tree-diff.c b/tree-diff.c\nindex ff4e0d3..edb4de9 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -607,7 +607,7 @@ static void try_to_follow_renames(const unsigned char *old, const unsigned char\n \tdiff_setup_done(&diff_opts);\n \tll_diff_tree_sha1(old, new, base, &diff_opts);\n \tdiffcore_std(&diff_opts);\n-\tfree_pathspec(&diff_opts.pathspec);\n+\tclear_pathspec(&diff_opts.pathspec);\n \n \t/* Go through the new set of filepairing, and see if we find a more interesting one */\n \topt->found_follow = 0;\n@@ -630,7 +630,7 @@ static void try_to_follow_renames(const unsigned char *old, const unsigned char\n \t\t\t/* Update the path we use from now on.. */\n \t\t\tpath[0] = p->one->path;\n \t\t\tpath[1] = NULL;\n-\t\t\tfree_pathspec(&opt->pathspec);\n+\t\t\tclear_pathspec(&opt->pathspec);\n \t\t\tparse_pathspec(&opt->pathspec,\n \t\t\t\t       PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL,\n \t\t\t\t       PATHSPEC_LITERAL_PATH, \"\", path);\n-- \n2.9.0-rc1-228-gd00d833\n"},{"id":"288159","messageId":"20160602212938.GA13071@sigill.intra.peff.net","threadId":"42512","inReplyTo":"xmqq60trthl4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: rename free_pathspec() to clear_pathspec()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-02T21:29:38Z","receivedAt":"2016-06-02T21:29:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 02, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:\n\n> The function takes a pointer to a pathspec structure, and releases\n> the resources held by it, but does not free() the structure itself.\n> Such a function should be called \"clear\", not \"free\".\n\nHmm, makes sense, though...\n\n>  * This is just something I noticed.  Among the hits in\n> \n>     $ git grep free_ \\*.h\n> \n>    I think free_notes() is also a candidate for such renaming, but\n>    because we are not actively working on that subsystem, we may\n>    want to leave that dog sleeping to avoid unnecessary code churn.\n>    The same for diff_free_filespec_data(), for which a better name\n>    would have been diff_filespec_clear().\n\nI think diff_filespec_clear() would not be quite right. It is freeing\nonly the allocated _data_, but leaving the other portions intact.\nGenerally our _clear() functions reset the object back to an initial\nstate, from which it can be reused. I don't see that as a big problem\nbecause there is an other object for the verb \"free\" here: \"data\". We\nare just freeing its data, but the rest of the object remains intact and\nwe may fill in the data again later.\n\nBut I think pathspec is in similar boat; it has not been cleared back to\nits initial state. But it is in a much _worse_ state than the filespec,\nwhich you can continue to use. It is in a totally broken state where\n\"nr\" does not correspond to the actual number of items, the has_wildcard\nflag is bogus, etc.\n\nSo I think it would be OK to move it to \"clear\", but we should probably\nalso zero the whole thing, too.\n\n-Peff\n"},{"id":"288167","messageId":"xmqqinxrs0yk.fsf@gitster.mtv.corp.google.com","threadId":"42512","inReplyTo":"20160602212938.GA13071@sigill.intra.peff.net","subject":"Re: [PATCH] pathspec: rename free_pathspec() to clear_pathspec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T22:03:15Z","receivedAt":"2016-06-02T22:03:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think diff_filespec_clear() would not be quite right. It is freeing\n> only the allocated _data_, but leaving the other portions intact.\n\nYou are (as usual) more right than I am ;-)\n\nYes, the free_data() is designed to retain enough information about\nthe filespec so that the data can be re-read by the next person who\nneeds to access it after free_data() is called; i.e. it was a\nmeasure to reduce the memory pressure by trading it off with I/O\ncost.  It is wrong to name it \"clear\", which is to \"clear the slate,\nmake it into pristine state\" whose side effect is to release held\nresources.\n"}]}