{"thread":{"id":"34012","subject":"[PATCH v2 0/7] unpack-trees: plug memory leak for merges","startedAt":"2013-06-02T15:46:50Z","lastAt":"2013-06-03T20:53:45Z","messageCount":22,"participants":["René Scharfe","Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"219168","messageId":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":null,"subject":"[PATCH v2 0/7] unpack-trees: plug memory leak for merges","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:50Z","receivedAt":"2013-06-02T15:46:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes in v2: Only changed the loop style in patch 7, as suggested\nby Felipe.\n\nThis series adds const to cache_entry pointers in a lot of places, in\norder to show that we can free them in unpack_nondirectories, which\nthe last patch finally does.\n\nFirst three easy patches for adding const and splitting a function in\ntwo:\n\n cache: mark cache_entry pointers const\n read-cache: mark cache_entry pointers const\n unpack-trees: factor out dup_entry\n\nThen a patch that reduces the side effects of merged_entry:\n\n unpack-trees: create working copy of merge entry in merged_entry\n\nAnother easy const patch:\n\n diff-lib, read-tree, unpack-trees: mark cache_entry pointers const\n\nAnd patch that introduces \"const struct cache_entry * const *\", which\nmay look a bit scary at first:\n\n diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const\n\nFinal patch that plugs a memory leak in unpack_nondirectories.\n\n unpack-trees: free cache_entry array members for merges\n\nIt's basically the same one that Stephen tested a while ago\n(http://thread.gmane.org/gmane.comp.version-control.git/222887).\n\nIt's not the only leak that affects cherry-pick; expect more\n(independent) patches.\n\n builtin/read-tree.c | 5 +-\n cache.h | 10 ++--\n diff-lib.c | 26 ++++-----\n read-cache.c | 18 ++++---\n unpack-trees.c | 148 ++++++++++++++++++++++++++++++++--------------------\n unpack-trees.h | 14 +++--\n 6 files changed, 133 insertions(+), 88 deletions(-)\n\n-- \n1.8.3\n"},{"id":"219169","messageId":"1370188017-24672-2-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"[PATCH v2 1/7] cache: mark cache_entry pointers const","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:51Z","receivedAt":"2013-06-02T15:46:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add const for pointers that are only dereferenced for reading by the\ninline functions copy_cache_entry and ce_mode_from_stat. This allows\ncallers to pass in const pointers.\n\nSigned-off-by: René Scharfe \n---\n cache.h | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 94ca1ac..43a27e7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -190,7 +190,8 @@ struct cache_entry {\n * another. But we never change the name, or the hash state!\n */\n #define CE_STATE_MASK (CE_HASHED | CE_UNHASHED)\n-static inline void copy_cache_entry(struct cache_entry *dst, struct cache_entry *src)\n+static inline void copy_cache_entry(struct cache_entry *dst,\n+\t\t\t\t const struct cache_entry *src)\n {\n \tunsigned int state = dst->ce_flags & CE_STATE_MASK;\n \n@@ -222,7 +223,8 @@ static inline unsigned int create_ce_mode(unsigned int mode)\n \t\treturn S_IFGITLINK;\n \treturn S_IFREG | ce_permissions(mode);\n }\n-static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned int mode)\n+static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,\n+\t\t\t\t\t unsigned int mode)\n {\n \textern int trust_executable_bit, has_symlinks;\n \tif (!has_symlinks && S_ISREG(mode) &&\n-- \n1.8.3\n"},{"id":"219171","messageId":"1370188017-24672-3-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"[PATCH v2 2/7] read-cache: mark cache_entry pointers const","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:52Z","receivedAt":"2013-06-02T15:46:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"ie_match_stat and ie_modified only derefence their struct cache_entry\npointers for reading. Add const to the parameter declaration here and\ndo the same for the static helper function used by them, as it's the\nsame there as well. This allows callers to pass in const pointers.\n\nSigned-off-by: René Scharfe \n---\n cache.h | 4 ++--\n read-cache.c | 18 ++++++++++--------\n 2 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 43a27e7..01e8760 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -482,8 +482,8 @@ extern void *read_blob_data_from_index(struct index_state *, const char *, unsig\n #define CE_MATCH_RACY_IS_DIRTY\t\t02\n /* do stat comparison even if CE_SKIP_WORKTREE is true */\n #define CE_MATCH_IGNORE_SKIP_WORKTREE\t04\n-extern int ie_match_stat(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);\n-extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);\n+extern int ie_match_stat(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n+extern int ie_modified(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);\n \n #define PATHSPEC_ONESTAR 1\t/* the pathspec pattern sastisfies GFNM_ONESTAR */\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 04ed561..e6e0466 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -91,7 +91,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n \t\tce_mark_uptodate(ce);\n }\n \n-static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n+static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n \tint fd = open(ce->name, O_RDONLY);\n@@ -105,7 +105,7 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n \treturn match;\n }\n \n-static int ce_compare_link(struct cache_entry *ce, size_t expected_size)\n+static int ce_compare_link(const struct cache_entry *ce, size_t expected_size)\n {\n \tint match = -1;\n \tvoid *buffer;\n@@ -126,7 +126,7 @@ static int ce_compare_link(struct cache_entry *ce, size_t expected_size)\n \treturn match;\n }\n \n-static int ce_compare_gitlink(struct cache_entry *ce)\n+static int ce_compare_gitlink(const struct cache_entry *ce)\n {\n \tunsigned char sha1[20];\n \n@@ -143,7 +143,7 @@ static int ce_compare_gitlink(struct cache_entry *ce)\n \treturn hashcmp(sha1, ce->sha1);\n }\n \n-static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)\n+static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)\n {\n \tswitch (st->st_mode & S_IFMT) {\n \tcase S_IFREG:\n@@ -163,7 +163,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)\n \treturn 0;\n }\n \n-static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n+static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)\n {\n \tunsigned int changed = 0;\n \n@@ -239,7 +239,8 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n \treturn changed;\n }\n \n-static int is_racy_timestamp(const struct index_state *istate, struct cache_entry *ce)\n+static int is_racy_timestamp(const struct index_state *istate,\n+\t\t\t const struct cache_entry *ce)\n {\n \treturn (!S_ISGITLINK(ce->ce_mode) &&\n \t\tistate->timestamp.sec &&\n@@ -255,7 +256,7 @@ static int is_racy_timestamp(const struct index_state *istate, struct cache_entr\n }\n \n int ie_match_stat(const struct index_state *istate,\n-\t\t struct cache_entry *ce, struct stat *st,\n+\t\t const struct cache_entry *ce, struct stat *st,\n \t\t unsigned int options)\n {\n \tunsigned int changed;\n@@ -311,7 +312,8 @@ int ie_match_stat(const struct index_state *istate,\n }\n \n int ie_modified(const struct index_state *istate,\n-\t\tstruct cache_entry *ce, struct stat *st, unsigned int options)\n+\t\tconst struct cache_entry *ce,\n+\t\tstruct stat *st, unsigned int options)\n {\n \tint changed, changed_fs;\n \n-- \n1.8.3\n"},{"id":"219173","messageId":"1370188017-24672-4-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"[PATCH v2 3/7] unpack-trees: factor out dup_entry","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:53Z","receivedAt":"2013-06-02T15:46:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"While we're add it, mark the struct cache_entry pointer of add_entry\nconst because we only read from it and this allows callers to pass in\nconst pointers.\n\nSigned-off-by: René Scharfe \n---\n unpack-trees.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ede4299..e8b4cc1 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -116,14 +116,20 @@ static void do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n \t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n }\n \n-static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n-\tunsigned int set, unsigned int clear)\n+static struct cache_entry *dup_entry(const struct cache_entry *ce)\n {\n \tunsigned int size = ce_size(ce);\n \tstruct cache_entry *new = xmalloc(size);\n \n \tmemcpy(new, ce, size);\n-\tdo_add_entry(o, new, set, clear);\n+\treturn new;\n+}\n+\n+static void add_entry(struct unpack_trees_options *o,\n+\t\t const struct cache_entry *ce,\n+\t\t unsigned int set, unsigned int clear)\n+{\n+\tdo_add_entry(o, dup_entry(ce), set, clear);\n }\n \n /*\n-- \n1.8.3\n"},{"id":"219170","messageId":"1370188017-24672-5-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"[PATCH v2 4/7] unpack-trees: create working copy of merge entry in merged_entry","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:54Z","receivedAt":"2013-06-02T15:46:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Duplicate the merge entry right away and work with that instead of\nmodifying the entry we got and duplicating it only at the end of\nthe function. Then mark that pointer const to document that we\ndon't modify the referenced cache_entry.\n\nThis change is safe because all existing merge functions call\nmerged_entry just before returning (or not at all), i.e. they don't\ncare about changes to the referenced cache_entry after the call.\nunpack_nondirectories and unpack_index_entry, which call the merge\nfunctions through call_unpack_fn, aren't interested in such changes\nneither.\n\nThe change complicates merged_entry a bit because we have to free the\ncopy if we error out, but allows callers to pass a const pointer.\n\nSigned-off-by: René Scharfe \n---\n unpack-trees.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e8b4cc1..2fecef8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1466,10 +1466,12 @@ static int verify_absent_sparse(struct cache_entry *ce,\n \treturn verify_absent_1(ce, orphaned_error, o);\n }\n \n-static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n-\t\tstruct unpack_trees_options *o)\n+static int merged_entry(const struct cache_entry *ce,\n+\t\t\tstruct cache_entry *old,\n+\t\t\tstruct unpack_trees_options *o)\n {\n \tint update = CE_UPDATE;\n+\tstruct cache_entry *merge = dup_entry(ce);\n \n \tif (!old) {\n \t\t/*\n@@ -1487,8 +1489,11 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\tupdate |= CE_ADDED;\n \t\tmerge->ce_flags |= CE_NEW_SKIP_WORKTREE;\n \n-\t\tif (verify_absent(merge, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))\n+\t\tif (verify_absent(merge,\n+\t\t\t\t ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) {\n+\t\t\tfree(merge);\n \t\t\treturn -1;\n+\t\t}\n \t\tinvalidate_ce_path(merge, o);\n \t} else if (!(old->ce_flags & CE_CONFLICTED)) {\n \t\t/*\n@@ -1502,8 +1507,10 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\t\tcopy_cache_entry(merge, old);\n \t\t\tupdate = 0;\n \t\t} else {\n-\t\t\tif (verify_uptodate(old, o))\n+\t\t\tif (verify_uptodate(old, o)) {\n+\t\t\t\tfree(merge);\n \t\t\t\treturn -1;\n+\t\t\t}\n \t\t\t/* Migrate old flags over */\n \t\t\tupdate |= old->ce_flags & (CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE);\n \t\t\tinvalidate_ce_path(old, o);\n@@ -1516,7 +1523,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\tinvalidate_ce_path(old, o);\n \t}\n \n-\tadd_entry(o, merge, update, CE_STAGEMASK);\n+\tdo_add_entry(o, merge, update, CE_STAGEMASK);\n \treturn 1;\n }\n \n-- \n1.8.3\n"},{"id":"219174","messageId":"1370188017-24672-6-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"[PATCH v2 5/7] diff-lib, read-tree, unpack-trees: mark cache_entry pointers const","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:55Z","receivedAt":"2013-06-02T15:46:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add const to struct cache_entry pointers throughout the tree which are\nonly used for reading. This allows callers to pass in const pointers.\n\nSigned-off-by: René Scharfe \n---\n builtin/read-tree.c | 2 +-\n diff-lib.c | 23 +++++++-------\n unpack-trees.c | 91 +++++++++++++++++++++++++++++------------------------\n 3 files changed, 63 insertions(+), 53 deletions(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 042ac1b..b847486 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -66,7 +66,7 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n-static void debug_stage(const char *label, struct cache_entry *ce,\n+static void debug_stage(const char *label, const struct cache_entry *ce,\n \t\t\tstruct unpack_trees_options *o)\n {\n \tprintf(\"%s \", label);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f35de0f..83d0cb8 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -64,8 +64,9 @@ static int check_removed(const struct cache_entry *ce, struct stat *st)\n * commits, untracked content and/or modified content).\n */\n static int match_stat_with_submodule(struct diff_options *diffopt,\n-\t\t\t\t struct cache_entry *ce, struct stat *st,\n-\t\t\t\t unsigned ce_option, unsigned *dirty_submodule)\n+\t\t\t\t const struct cache_entry *ce,\n+\t\t\t\t struct stat *st, unsigned ce_option,\n+\t\t\t\t unsigned *dirty_submodule)\n {\n \tint changed = ce_match_stat(ce, st, ce_option);\n \tif (S_ISGITLINK(ce->ce_mode)) {\n@@ -237,7 +238,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n /* A file entry went away or appeared */\n static void diff_index_show_file(struct rev_info *revs,\n \t\t\t\t const char *prefix,\n-\t\t\t\t struct cache_entry *ce,\n+\t\t\t\t const struct cache_entry *ce,\n \t\t\t\t const unsigned char *sha1, int sha1_valid,\n \t\t\t\t unsigned int mode,\n \t\t\t\t unsigned dirty_submodule)\n@@ -246,7 +247,7 @@ static void diff_index_show_file(struct rev_info *revs,\n \t\t sha1, sha1_valid, ce->name, dirty_submodule);\n }\n \n-static int get_stat_data(struct cache_entry *ce,\n+static int get_stat_data(const struct cache_entry *ce,\n \t\t\t const unsigned char **sha1p,\n \t\t\t unsigned int *modep,\n \t\t\t int cached, int match_missing,\n@@ -283,7 +284,7 @@ static int get_stat_data(struct cache_entry *ce,\n }\n \n static void show_new_file(struct rev_info *revs,\n-\t\t\t struct cache_entry *new,\n+\t\t\t const struct cache_entry *new,\n \t\t\t int cached, int match_missing)\n {\n \tconst unsigned char *sha1;\n@@ -302,8 +303,8 @@ static void show_new_file(struct rev_info *revs,\n }\n \n static int show_modified(struct rev_info *revs,\n-\t\t\t struct cache_entry *old,\n-\t\t\t struct cache_entry *new,\n+\t\t\t const struct cache_entry *old,\n+\t\t\t const struct cache_entry *new,\n \t\t\t int report_missing,\n \t\t\t int cached, int match_missing)\n {\n@@ -362,8 +363,8 @@ static int show_modified(struct rev_info *revs,\n * give you the position and number of entries in the index).\n */\n static void do_oneway_diff(struct unpack_trees_options *o,\n-\tstruct cache_entry *idx,\n-\tstruct cache_entry *tree)\n+\t\t\t const struct cache_entry *idx,\n+\t\t\t const struct cache_entry *tree)\n {\n \tstruct rev_info *revs = o->unpack_data;\n \tint match_missing, cached;\n@@ -425,8 +426,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n */\n static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)\n {\n-\tstruct cache_entry *idx = src[0];\n-\tstruct cache_entry *tree = src[1];\n+\tconst struct cache_entry *idx = src[0];\n+\tconst struct cache_entry *tree = src[1];\n \tstruct rev_info *revs = o->unpack_data;\n \n \t/*\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2fecef8..c5a40df 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -241,8 +241,11 @@ static int check_updates(struct unpack_trees_options *o)\n \treturn errs != 0;\n }\n \n-static int verify_uptodate_sparse(struct cache_entry *ce, struct unpack_trees_options *o);\n-static int verify_absent_sparse(struct cache_entry *ce, enum unpack_trees_error_types, struct unpack_trees_options *o);\n+static int verify_uptodate_sparse(const struct cache_entry *ce,\n+\t\t\t\t struct unpack_trees_options *o);\n+static int verify_absent_sparse(const struct cache_entry *ce,\n+\t\t\t\tenum unpack_trees_error_types,\n+\t\t\t\tstruct unpack_trees_options *o);\n \n static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_options *o)\n {\n@@ -326,7 +329,7 @@ static void mark_all_ce_unused(struct index_state *index)\n \t\tindex->cache[i]->ce_flags &= ~(CE_UNPACKED | CE_ADDED | CE_NEW_SKIP_WORKTREE);\n }\n \n-static int locate_in_src_index(struct cache_entry *ce,\n+static int locate_in_src_index(const struct cache_entry *ce,\n \t\t\t struct unpack_trees_options *o)\n {\n \tstruct index_state *index = o->src_index;\n@@ -1001,7 +1004,9 @@ static void mark_new_skip_worktree(struct exclude_list *el,\n \t\t select_flag, skip_wt_flag, el);\n }\n \n-static int verify_absent(struct cache_entry *, enum unpack_trees_error_types, struct unpack_trees_options *);\n+static int verify_absent(const struct cache_entry *,\n+\t\t\t enum unpack_trees_error_types,\n+\t\t\t struct unpack_trees_options *);\n /*\n * N-way merge \"len\" trees. Returns 0 on success, -1 on failure to manipulate the\n * resulting index, -2 on failure to reflect the changes to the work tree.\n@@ -1171,12 +1176,13 @@ return_failed:\n \n /* Here come the merge functions */\n \n-static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)\n+static int reject_merge(const struct cache_entry *ce,\n+\t\t\tstruct unpack_trees_options *o)\n {\n \treturn add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);\n }\n \n-static int same(struct cache_entry *a, struct cache_entry *b)\n+static int same(const struct cache_entry *a, const struct cache_entry *b)\n {\n \tif (!!a != !!b)\n \t\treturn 0;\n@@ -1193,9 +1199,9 @@ static int same(struct cache_entry *a, struct cache_entry *b)\n * When a CE gets turned into an unmerged entry, we\n * want it to be up-to-date\n */\n-static int verify_uptodate_1(struct cache_entry *ce,\n-\t\t\t\t struct unpack_trees_options *o,\n-\t\t\t\t enum unpack_trees_error_types error_type)\n+static int verify_uptodate_1(const struct cache_entry *ce,\n+\t\t\t struct unpack_trees_options *o,\n+\t\t\t enum unpack_trees_error_types error_type)\n {\n \tstruct stat st;\n \n@@ -1234,7 +1240,7 @@ static int verify_uptodate_1(struct cache_entry *ce,\n \t\tadd_rejected_path(o, error_type, ce->name);\n }\n \n-static int verify_uptodate(struct cache_entry *ce,\n+static int verify_uptodate(const struct cache_entry *ce,\n \t\t\t struct unpack_trees_options *o)\n {\n \tif (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))\n@@ -1242,13 +1248,14 @@ static int verify_uptodate(struct cache_entry *ce,\n \treturn verify_uptodate_1(ce, o, ERROR_NOT_UPTODATE_FILE);\n }\n \n-static int verify_uptodate_sparse(struct cache_entry *ce,\n+static int verify_uptodate_sparse(const struct cache_entry *ce,\n \t\t\t\t struct unpack_trees_options *o)\n {\n \treturn verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE);\n }\n \n-static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_options *o)\n+static void invalidate_ce_path(const struct cache_entry *ce,\n+\t\t\t struct unpack_trees_options *o)\n {\n \tif (ce)\n \t\tcache_tree_invalidate_path(o->src_index->cache_tree, ce->name);\n@@ -1261,16 +1268,16 @@ static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_optio\n * Currently, git does not checkout subprojects during a superproject\n * checkout, so it is not going to overwrite anything.\n */\n-static int verify_clean_submodule(struct cache_entry *ce,\n-\t\t\t\t enum unpack_trees_error_types error_type,\n-\t\t\t\t struct unpack_trees_options *o)\n+static int verify_clean_submodule(const struct cache_entry *ce,\n+\t\t\t\t enum unpack_trees_error_types error_type,\n+\t\t\t\t struct unpack_trees_options *o)\n {\n \treturn 0;\n }\n \n-static int verify_clean_subdirectory(struct cache_entry *ce,\n-\t\t\t\t enum unpack_trees_error_types error_type,\n-\t\t\t\t struct unpack_trees_options *o)\n+static int verify_clean_subdirectory(const struct cache_entry *ce,\n+\t\t\t\t enum unpack_trees_error_types error_type,\n+\t\t\t\t struct unpack_trees_options *o)\n {\n \t/*\n \t * we are about to extract \"ce->name\"; we would not want to lose\n@@ -1356,7 +1363,7 @@ static int icase_exists(struct unpack_trees_options *o, const char *name, int le\n }\n \n static int check_ok_to_remove(const char *name, int len, int dtype,\n-\t\t\t struct cache_entry *ce, struct stat *st,\n+\t\t\t const struct cache_entry *ce, struct stat *st,\n \t\t\t enum unpack_trees_error_types error_type,\n \t\t\t struct unpack_trees_options *o)\n {\n@@ -1411,9 +1418,9 @@ static int check_ok_to_remove(const char *name, int len, int dtype,\n * We do not want to remove or overwrite a working tree file that\n * is not tracked, unless it is ignored.\n */\n-static int verify_absent_1(struct cache_entry *ce,\n-\t\t\t\t enum unpack_trees_error_types error_type,\n-\t\t\t\t struct unpack_trees_options *o)\n+static int verify_absent_1(const struct cache_entry *ce,\n+\t\t\t enum unpack_trees_error_types error_type,\n+\t\t\t struct unpack_trees_options *o)\n {\n \tint len;\n \tstruct stat st;\n@@ -1446,7 +1453,7 @@ static int verify_absent_1(struct cache_entry *ce,\n \t}\n }\n \n-static int verify_absent(struct cache_entry *ce,\n+static int verify_absent(const struct cache_entry *ce,\n \t\t\t enum unpack_trees_error_types error_type,\n \t\t\t struct unpack_trees_options *o)\n {\n@@ -1455,9 +1462,9 @@ static int verify_absent(struct cache_entry *ce,\n \treturn verify_absent_1(ce, error_type, o);\n }\n \n-static int verify_absent_sparse(struct cache_entry *ce,\n-\t\t\t enum unpack_trees_error_types error_type,\n-\t\t\t struct unpack_trees_options *o)\n+static int verify_absent_sparse(const struct cache_entry *ce,\n+\t\t\t\tenum unpack_trees_error_types error_type,\n+\t\t\t\tstruct unpack_trees_options *o)\n {\n \tenum unpack_trees_error_types orphaned_error = error_type;\n \tif (orphaned_error == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN)\n@@ -1467,7 +1474,7 @@ static int verify_absent_sparse(struct cache_entry *ce,\n }\n \n static int merged_entry(const struct cache_entry *ce,\n-\t\t\tstruct cache_entry *old,\n+\t\t\tconst struct cache_entry *old,\n \t\t\tstruct unpack_trees_options *o)\n {\n \tint update = CE_UPDATE;\n@@ -1527,8 +1534,9 @@ static int merged_entry(const struct cache_entry *ce,\n \treturn 1;\n }\n \n-static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,\n-\t\tstruct unpack_trees_options *o)\n+static int deleted_entry(const struct cache_entry *ce,\n+\t\t\t const struct cache_entry *old,\n+\t\t\t struct unpack_trees_options *o)\n {\n \t/* Did it exist in the index? */\n \tif (!old) {\n@@ -1543,7 +1551,8 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,\n \treturn 1;\n }\n \n-static int keep_entry(struct cache_entry *ce, struct unpack_trees_options *o)\n+static int keep_entry(const struct cache_entry *ce,\n+\t\t struct unpack_trees_options *o)\n {\n \tadd_entry(o, ce, 0, 0);\n \treturn 1;\n@@ -1567,9 +1576,9 @@ static void show_stage_entry(FILE *o,\n \n int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n {\n-\tstruct cache_entry *index;\n-\tstruct cache_entry *head;\n-\tstruct cache_entry *remote = stages[o->head_idx + 1];\n+\tconst struct cache_entry *index;\n+\tconst struct cache_entry *head;\n+\tconst struct cache_entry *remote = stages[o->head_idx + 1];\n \tint count;\n \tint head_match = 0;\n \tint remote_match = 0;\n@@ -1654,7 +1663,7 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n \tif (o->aggressive) {\n \t\tint head_deleted = !head;\n \t\tint remote_deleted = !remote;\n-\t\tstruct cache_entry *ce = NULL;\n+\t\tconst struct cache_entry *ce = NULL;\n \n \t\tif (index)\n \t\t\tce = index;\n@@ -1739,9 +1748,9 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n */\n int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n {\n-\tstruct cache_entry *current = src[0];\n-\tstruct cache_entry *oldtree = src[1];\n-\tstruct cache_entry *newtree = src[2];\n+\tconst struct cache_entry *current = src[0];\n+\tconst struct cache_entry *oldtree = src[1];\n+\tconst struct cache_entry *newtree = src[2];\n \n \tif (o->merge_size != 2)\n \t\treturn error(\"Cannot do a twoway merge of %d trees\",\n@@ -1806,8 +1815,8 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n int bind_merge(struct cache_entry **src,\n \t\tstruct unpack_trees_options *o)\n {\n-\tstruct cache_entry *old = src[0];\n-\tstruct cache_entry *a = src[1];\n+\tconst struct cache_entry *old = src[0];\n+\tconst struct cache_entry *a = src[1];\n \n \tif (o->merge_size != 1)\n \t\treturn error(\"Cannot do a bind merge of %d trees\",\n@@ -1829,8 +1838,8 @@ int bind_merge(struct cache_entry **src,\n */\n int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n {\n-\tstruct cache_entry *old = src[0];\n-\tstruct cache_entry *a = src[1];\n+\tconst struct cache_entry *old = src[0];\n+\tconst struct cache_entry *a = src[1];\n \n \tif (o->merge_size != 1)\n \t\treturn error(\"Cannot do a oneway merge of %d trees\",\n-- \n1.8.3\n"},{"id":"219172","messageId":"1370188017-24672-7-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"[PATCH v2 6/7] diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:56Z","receivedAt":"2013-06-02T15:46:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Change the type merge_fn_t to accept the array of cache_entry pointers\nas const pointers to const pointers. This documents the fact that the\nmerge functions don't modify the cache_entry contents or replace any of\nthe pointers in the array.\n\nOnly a single cast is necessary in unpack_nondirectories because adding\ntwo const modifiers at once is not allowed in C. The cast is safe in\nthat it doesn't mask any modfication; call_unpack_fn only needs the\narray for reading.\n\nSigned-off-by: René Scharfe \n---\n builtin/read-tree.c | 3 ++-\n diff-lib.c | 3 ++-\n unpack-trees.c | 21 +++++++++++++--------\n unpack-trees.h | 14 +++++++++-----\n 4 files changed, 26 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex b847486..0f5d7fe 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -80,7 +80,8 @@ static void debug_stage(const char *label, const struct cache_entry *ce,\n \t\t sha1_to_hex(ce->sha1));\n }\n \n-static int debug_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n+static int debug_merge(const struct cache_entry * const *stages,\n+\t\t struct unpack_trees_options *o)\n {\n \tint i;\n \ndiff --git a/diff-lib.c b/diff-lib.c\nindex 83d0cb8..b6f4b21 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -424,7 +424,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n * the fairly complex unpack_trees() semantic requirements, including\n * the skipping, the path matching, the type conflict cases etc.\n */\n-static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)\n+static int oneway_diff(const struct cache_entry * const *src,\n+\t\t struct unpack_trees_options *o)\n {\n \tconst struct cache_entry *idx = src[0];\n \tconst struct cache_entry *tree = src[1];\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex c5a40df..2dbc05d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -300,7 +300,8 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt\n \treturn 0;\n }\n \n-static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_options *o)\n+static inline int call_unpack_fn(const struct cache_entry * const *src,\n+\t\t\t\t struct unpack_trees_options *o)\n {\n \tint ret = o->fn(src, o);\n \tif (ret > 0)\n@@ -397,7 +398,7 @@ static void add_same_unmerged(struct cache_entry *ce,\n static int unpack_index_entry(struct cache_entry *ce,\n \t\t\t struct unpack_trees_options *o)\n {\n-\tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n+\tconst struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n \tint ret;\n \n \tsrc[0] = ce;\n@@ -600,7 +601,8 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \t}\n \n \tif (o->merge)\n-\t\treturn call_unpack_fn(src, o);\n+\t\treturn call_unpack_fn((const struct cache_entry * const *)src,\n+\t\t\t\t o);\n \n \tfor (i = 0; i < n; i++)\n \t\tif (src[i] && src[i] != o->df_conflict_entry)\n@@ -1574,7 +1576,8 @@ static void show_stage_entry(FILE *o,\n }\n #endif\n \n-int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n+int threeway_merge(const struct cache_entry * const *stages,\n+\t\t struct unpack_trees_options *o)\n {\n \tconst struct cache_entry *index;\n \tconst struct cache_entry *head;\n@@ -1746,7 +1749,8 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n * \"carry forward\" rule, please see .\n *\n */\n-int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n+int twoway_merge(const struct cache_entry * const *src,\n+\t\t struct unpack_trees_options *o)\n {\n \tconst struct cache_entry *current = src[0];\n \tconst struct cache_entry *oldtree = src[1];\n@@ -1812,8 +1816,8 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n * Keep the index entries at stage0, collapse stage1 but make sure\n * stage0 does not have anything there.\n */\n-int bind_merge(struct cache_entry **src,\n-\t\tstruct unpack_trees_options *o)\n+int bind_merge(const struct cache_entry * const *src,\n+\t struct unpack_trees_options *o)\n {\n \tconst struct cache_entry *old = src[0];\n \tconst struct cache_entry *a = src[1];\n@@ -1836,7 +1840,8 @@ int bind_merge(struct cache_entry **src,\n * The rule is:\n * - take the stat information from stage0, take the data from stage1\n */\n-int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n+int oneway_merge(const struct cache_entry * const *src,\n+\t\t struct unpack_trees_options *o)\n {\n \tconst struct cache_entry *old = src[0];\n \tconst struct cache_entry *a = src[1];\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 5e432f5..36a73a6 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -8,7 +8,7 @@\n struct unpack_trees_options;\n struct exclude_list;\n \n-typedef int (*merge_fn_t)(struct cache_entry **src,\n+typedef int (*merge_fn_t)(const struct cache_entry * const *src,\n \t\tstruct unpack_trees_options *options);\n \n enum unpack_trees_error_types {\n@@ -77,9 +77,13 @@ struct unpack_trees_options {\n extern int unpack_trees(unsigned n, struct tree_desc *t,\n \t\tstruct unpack_trees_options *options);\n \n-int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o);\n-int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o);\n-int bind_merge(struct cache_entry **src, struct unpack_trees_options *o);\n-int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o);\n+int threeway_merge(const struct cache_entry * const *stages,\n+\t\t struct unpack_trees_options *o);\n+int twoway_merge(const struct cache_entry * const *src,\n+\t\t struct unpack_trees_options *o);\n+int bind_merge(const struct cache_entry * const *src,\n+\t struct unpack_trees_options *o);\n+int oneway_merge(const struct cache_entry * const *src,\n+\t\t struct unpack_trees_options *o);\n \n #endif\n-- \n1.8.3\n"},{"id":"219175","messageId":"1370188017-24672-8-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34012","inReplyTo":"1370188017-24672-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"[PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T15:46:57Z","receivedAt":"2013-06-02T15:46:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The merge functions duplicate entries as needed and they don't free\nthem. Release them in unpack_nondirectories, the same function\nwhere they were allocated, after we're done.\n\nAs suggested by Felipe, use the same loop style (zero-based for loop)\nfor freeing as for allocating.\n\nImproved-by: Felipe Contreras \nSigned-off-by: René Scharfe \n---\n unpack-trees.c | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2dbc05d..57b4074 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -600,9 +600,16 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \t\tsrc[i + o->merge] = create_ce_entry(info, names + i, stage);\n \t}\n \n-\tif (o->merge)\n-\t\treturn call_unpack_fn((const struct cache_entry * const *)src,\n-\t\t\t\t o);\n+\tif (o->merge) {\n+\t\tint rc = call_unpack_fn((const struct cache_entry * const *)src,\n+\t\t\t\t\to);\n+\t\tfor (i = 0; i < n; i++) {\n+\t\t\tstruct cache_entry *ce = src[i + o->merge];\n+\t\t\tif (ce != o->df_conflict_entry)\n+\t\t\t\tfree(ce);\n+\t\t}\n+\t\treturn rc;\n+\t}\n \n \tfor (i = 0; i < n; i++)\n \t\tif (src[i] && src[i] != o->df_conflict_entry)\n-- \n1.8.3\n"},{"id":"219177","messageId":"CAMP44s2ym5UEPo8kr6YKf1x_P_0L+o_9vWdCTA6MPDQsRy7d1A@mail.gmail.com","threadId":"34012","inReplyTo":"1370188017-24672-8-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-02T17:25:08Z","receivedAt":"2013-06-02T17:25:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe\n wrote:\n> The merge functions duplicate entries as needed and they don't free\n> them. Release them in unpack_nondirectories, the same function\n> where they were allocated, after we're done.\n>\n> As suggested by Felipe, use the same loop style (zero-based for loop)\n> for freeing as for allocating.\n>\n> Improved-by: Felipe Contreras \n> Signed-off-by: René Scharfe \n> ---\n> unpack-trees.c | 13 ++++++++++---\n> 1 file changed, 10 insertions(+), 3 deletions(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 2dbc05d..57b4074 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -600,9 +600,16 @@ static int unpack_nondirectories(int n, unsigned long mask,\n> src[i + o->merge] = create_ce_entry(info, names + i, stage);\n> }\n>\n> - if (o->merge)\n> - return call_unpack_fn((const struct cache_entry * const *)src,\n> - o);\n> + if (o->merge) {\n> + int rc = call_unpack_fn((const struct cache_entry * const *)src,\n> + o);\n> + for (i = 0; i < n; i++) {\n> + struct cache_entry *ce = src[i + o->merge];\n> + if (ce != o->df_conflict_entry)\n\nIt's possible that ce is NULL, but you didn't add that check because\nfree(NULL) still works? Or because ce cannot be NULL?\n\nIf it's the former, I think it's clearer if we check that ce is not\nNULL either way.\n\nOtherwise it's OK by me.\n\n> + free(ce);\n> + }\n> + return rc;\n> + }\n\n-- \nFelipe Contreras\n"},{"id":"219179","messageId":"51AB86BB.3080203@lsrfire.ath.cx","threadId":"34012","inReplyTo":"CAMP44s2ym5UEPo8kr6YKf1x_P_0L+o_9vWdCTA6MPDQsRy7d1A@mail.gmail.com","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T17:54:03Z","receivedAt":"2013-06-02T17:54:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.06.2013 19:25, schrieb Felipe Contreras:\n> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe\n> wrote:\n>> + for (i = 0; i < n; i++) {\n>> + struct cache_entry *ce = src[i + o->merge];\n>> + if (ce != o->df_conflict_entry)\n>\n> It's possible that ce is NULL, but you didn't add that check because\n> free(NULL) still works? Or because ce cannot be NULL?\n>\n> If it's the former, I think it's clearer if we check that ce is not\n> NULL either way.\n\nIt is NULL if one tree misses an entry (e.g. a new or removed file). \nfree handles NULL and we generally avoid duplicating its NULL-check.\n\nYou're probably referring to the non-merge case as the example for \nchecking. That one is different, though, because there we call \ndo_add_entry instead of free, which does not handle NULL. And it \ndoesn't have to, as it is mostly called through add_entry, which never \npasses NULL.\n\nRené\n"},{"id":"219180","messageId":"CAMP44s0Af90Sfi47_mxEGRFHAsAhaoO6T1jjbj7SptucNO-k0w@mail.gmail.com","threadId":"34012","inReplyTo":"51AB86BB.3080203@lsrfire.ath.cx","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-02T17:59:26Z","receivedAt":"2013-06-02T17:59:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe\n wrote:\n> Am 02.06.2013 19:25, schrieb Felipe Contreras:\n>>\n>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe\n>> wrote:\n>>>\n>>> + for (i = 0; i < n; i++) {\n>>> + struct cache_entry *ce = src[i + o->merge];\n>>> + if (ce != o->df_conflict_entry)\n>>\n>>\n>> It's possible that ce is NULL, but you didn't add that check because\n>> free(NULL) still works? Or because ce cannot be NULL?\n>>\n>> If it's the former, I think it's clearer if we check that ce is not\n>> NULL either way.\n>\n>\n> It is NULL if one tree misses an entry (e.g. a new or removed file). free\n> handles NULL and we generally avoid duplicating its NULL-check.\n\nYeah, but I can see somebody adding code inside that 'if' clause to\nprint the cache entry, and see a crash only to wonder what's going on.\nAnd to save what? 5 characters?\n\n-- \nFelipe Contreras\n"},{"id":"219190","messageId":"51ABAA84.8090301@lsrfire.ath.cx","threadId":"34012","inReplyTo":"CAMP44s0Af90Sfi47_mxEGRFHAsAhaoO6T1jjbj7SptucNO-k0w@mail.gmail.com","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T20:26:44Z","receivedAt":"2013-06-02T20:26:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.06.2013 19:59, schrieb Felipe Contreras:\n> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe\n> wrote:\n>> Am 02.06.2013 19:25, schrieb Felipe Contreras:\n>>>\n>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe\n>>> wrote:\n>>>>\n>>>> + for (i = 0; i < n; i++) {\n>>>> + struct cache_entry *ce = src[i + o->merge];\n>>>> + if (ce != o->df_conflict_entry)\n>>>\n>>>\n>>> It's possible that ce is NULL, but you didn't add that check because\n>>> free(NULL) still works? Or because ce cannot be NULL?\n>>>\n>>> If it's the former, I think it's clearer if we check that ce is not\n>>> NULL either way.\n>>\n>>\n>> It is NULL if one tree misses an entry (e.g. a new or removed file). free\n>> handles NULL and we generally avoid duplicating its NULL-check.\n>\n> Yeah, but I can see somebody adding code inside that 'if' clause to\n> print the cache entry, and see a crash only to wonder what's going on.\n> And to save what? 5 characters?\n\nThe person adding code that depends on ce not being NULL needs to add \nthat check as well. Let's not worry too much about future changes that \nmay or (more likely IMHO) may not be done. The test suite covers this \ncase multiple times, so such a mistake doesn't have a realistic chance \nto hit master.\n\nRené\n"},{"id":"219197","messageId":"CAMP44s3cqa-jETHX+ftbAVMx+oV6PMcVkdH63P93ER-4fH28Hw@mail.gmail.com","threadId":"34012","inReplyTo":"51ABAA84.8090301@lsrfire.ath.cx","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-02T22:38:38Z","receivedAt":"2013-06-02T22:38:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe\n wrote:\n> Am 02.06.2013 19:59, schrieb Felipe Contreras:\n>\n>> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe\n>> wrote:\n>>>\n>>> Am 02.06.2013 19:25, schrieb Felipe Contreras:\n>>>>\n>>>>\n>>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe\n>>>> wrote:\n>>>>>\n>>>>>\n>>>>> + for (i = 0; i < n; i++) {\n>>>>> + struct cache_entry *ce = src[i + o->merge];\n>>>>> + if (ce != o->df_conflict_entry)\n>>>>\n>>>>\n>>>>\n>>>> It's possible that ce is NULL, but you didn't add that check because\n>>>> free(NULL) still works? Or because ce cannot be NULL?\n>>>>\n>>>> If it's the former, I think it's clearer if we check that ce is not\n>>>> NULL either way.\n>>>\n>>>\n>>>\n>>> It is NULL if one tree misses an entry (e.g. a new or removed file). free\n>>> handles NULL and we generally avoid duplicating its NULL-check.\n>>\n>>\n>> Yeah, but I can see somebody adding code inside that 'if' clause to\n>> print the cache entry, and see a crash only to wonder what's going on.\n>> And to save what? 5 characters?\n>\n>\n> The person adding code that depends on ce not being NULL needs to add that\n> check as well. Let's not worry too much about future changes that may or\n> (more likely IMHO) may not be done. The test suite covers this case\n> multiple times, so such a mistake doesn't have a realistic chance to hit\n> master.\n\nWhat do we gain by not doing this? 5 less characters?\n\n-- \nFelipe Contreras\n"},{"id":"219201","messageId":"51ABD00C.7080503@lsrfire.ath.cx","threadId":"34012","inReplyTo":"CAMP44s3cqa-jETHX+ftbAVMx+oV6PMcVkdH63P93ER-4fH28Hw@mail.gmail.com","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T23:06:52Z","receivedAt":"2013-06-02T23:06:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.2013 00:38, schrieb Felipe Contreras:\n> On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe\n> wrote:\n>> Am 02.06.2013 19:59, schrieb Felipe Contreras:\n>>\n>>> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe\n>>> wrote:\n>>>>\n>>>> Am 02.06.2013 19:25, schrieb Felipe Contreras:\n>>>>>\n>>>>>\n>>>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe\n>>>>> wrote:\n>>>>>>\n>>>>>>\n>>>>>> + for (i = 0; i < n; i++) {\n>>>>>> + struct cache_entry *ce = src[i + o->merge];\n>>>>>> + if (ce != o->df_conflict_entry)\n>>>>>\n>>>>>\n>>>>>\n>>>>> It's possible that ce is NULL, but you didn't add that check because\n>>>>> free(NULL) still works? Or because ce cannot be NULL?\n>>>>>\n>>>>> If it's the former, I think it's clearer if we check that ce is not\n>>>>> NULL either way.\n>>>>\n>>>>\n>>>>\n>>>> It is NULL if one tree misses an entry (e.g. a new or removed file). free\n>>>> handles NULL and we generally avoid duplicating its NULL-check.\n>>>\n>>>\n>>> Yeah, but I can see somebody adding code inside that 'if' clause to\n>>> print the cache entry, and see a crash only to wonder what's going on.\n>>> And to save what? 5 characters?\n>>\n>>\n>> The person adding code that depends on ce not being NULL needs to add that\n>> check as well. Let's not worry too much about future changes that may or\n>> (more likely IMHO) may not be done. The test suite covers this case\n>> multiple times, so such a mistake doesn't have a realistic chance to hit\n>> master.\n>\n> What do we gain by not doing this? 5 less characters?\n\nBy following the convention of not checking for NULL when freeing, we \nreduce the size of the code slightly and have one less branch \ninstruction in the object code. That's not particularly exciting in a \nsingle instance but makes a difference if followed throughout the code base.\n\nWhat do we gain by adding a duplicate check? A few minutes of debugging \ntime by the person who will add some code and forget the NULL check? \nAnd how likely is that to happen?\n\nRené\n"},{"id":"219202","messageId":"CAMP44s2+Hx-6E7DkmWL_m92jDt5-Cj8FGHpACBZcqp1sn5Tfzw@mail.gmail.com","threadId":"34012","inReplyTo":"51ABD00C.7080503@lsrfire.ath.cx","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-02T23:23:12Z","receivedAt":"2013-06-02T23:23:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 2, 2013 at 6:06 PM, René Scharfe\n wrote:\n> Am 03.06.2013 00:38, schrieb Felipe Contreras:\n>\n>> On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe\n>> wrote:\n>>>\n>>> Am 02.06.2013 19:59, schrieb Felipe Contreras:\n>>>\n>>>> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe\n>>>> wrote:\n>>>>>\n>>>>>\n>>>>> Am 02.06.2013 19:25, schrieb Felipe Contreras:\n>>>>>>\n>>>>>>\n>>>>>>\n>>>>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe\n>>>>>> wrote:\n>>>>>>>\n>>>>>>>\n>>>>>>>\n>>>>>>> + for (i = 0; i < n; i++) {\n>>>>>>> + struct cache_entry *ce = src[i + o->merge];\n>>>>>>> + if (ce != o->df_conflict_entry)\n>>>>>>\n>>>>>>\n>>>>>>\n>>>>>>\n>>>>>> It's possible that ce is NULL, but you didn't add that check because\n>>>>>> free(NULL) still works? Or because ce cannot be NULL?\n>>>>>>\n>>>>>> If it's the former, I think it's clearer if we check that ce is not\n>>>>>> NULL either way.\n>>>>>\n>>>>>\n>>>>>\n>>>>>\n>>>>> It is NULL if one tree misses an entry (e.g. a new or removed file).\n>>>>> free\n>>>>> handles NULL and we generally avoid duplicating its NULL-check.\n>>>>\n>>>>\n>>>>\n>>>> Yeah, but I can see somebody adding code inside that 'if' clause to\n>>>> print the cache entry, and see a crash only to wonder what's going on.\n>>>> And to save what? 5 characters?\n>>>\n>>>\n>>>\n>>> The person adding code that depends on ce not being NULL needs to add\n>>> that\n>>> check as well. Let's not worry too much about future changes that may or\n>>> (more likely IMHO) may not be done. The test suite covers this case\n>>> multiple times, so such a mistake doesn't have a realistic chance to hit\n>>> master.\n>>\n>>\n>> What do we gain by not doing this? 5 less characters?\n>\n>\n> By following the convention of not checking for NULL when freeing,\n\nThat's not what I asked.\n\nI didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I\nsaid we should do 'if (cd && ce != o->df_conflict_entry)' instead of\n'if (ce != o->df_conflict_entry)'.\n\nThere's no reason not to.\n\n-- \nFelipe Contreras\n"},{"id":"219206","messageId":"51ABD99A.2030303@lsrfire.ath.cx","threadId":"34012","inReplyTo":"CAMP44s2+Hx-6E7DkmWL_m92jDt5-Cj8FGHpACBZcqp1sn5Tfzw@mail.gmail.com","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-02T23:47:38Z","receivedAt":"2013-06-02T23:47:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.2013 01:23, schrieb Felipe Contreras:\n> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I\n> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of\n> 'if (ce != o->df_conflict_entry)'.\n\nI did assume you meant the latter.\n\n> There's no reason not to.\n\nOnly the minor ones already mentioned: More text, one more branch in \nobject code, no benefit except for some hypothetical future case that's \ncaught by the test suite anyway -- or by code review.\n\nI wonder if we already reached the point where we spent more time \ndiscussing this change than the time needed by the envisioned developer \nto find and fix the NULL check that suddenly became necessary. :)\n\nRené\n"},{"id":"219208","messageId":"CAMP44s2HotyK7hh0nFjCe+BBtu1pyqmkn6=yt9WV6UyhxjrXoQ@mail.gmail.com","threadId":"34012","inReplyTo":"51ABD99A.2030303@lsrfire.ath.cx","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-03T00:04:55Z","receivedAt":"2013-06-03T00:04:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe\n wrote:\n> Am 03.06.2013 01:23, schrieb Felipe Contreras:\n>\n>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I\n>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of\n>> 'if (ce != o->df_conflict_entry)'.\n>\n>\n> I did assume you meant the latter.\n>\n>\n>> There's no reason not to.\n>\n>\n> Only the minor ones already mentioned: More text,\n\nFive characters.\n\n> one more branch in object\n> code,\n\nWhich might actually be more optimal.\n\n> no benefit except for some hypothetical future case that's caught by\n> the test suite anyway -- or by code review.\n\nThat's not the benefit, the benefit is that the code is clearer.\n\n> I wonder if we already reached the point where we spent more time discussing\n> this change than the time needed by the envisioned developer to find and fix\n> the NULL check that suddenly became necessary. :)\n\nMaybe, but if what you want is to avoid the discussion, you could just\nadd the extra five characters and be done with it.\n\n-- \nFelipe Contreras\n"},{"id":"219235","messageId":"51ACBD68.4070506@lsrfire.ath.cx","threadId":"34012","inReplyTo":"CAMP44s2HotyK7hh0nFjCe+BBtu1pyqmkn6=yt9WV6UyhxjrXoQ@mail.gmail.com","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-03T15:59:36Z","receivedAt":"2013-06-03T15:59:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.2013 02:04, schrieb Felipe Contreras:\n> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe\n> wrote:\n>> Am 03.06.2013 01:23, schrieb Felipe Contreras:\n>>\n>>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I\n>>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of\n>>> 'if (ce != o->df_conflict_entry)'.\n>>\n>>\n>> I did assume you meant the latter.\n>>\n>>\n>>> There's no reason not to.\n>>\n>>\n>> Only the minor ones already mentioned: More text,\n>\n> Five characters.\n>\n>> one more branch in object\n>> code,\n>\n> Which might actually be more optimal.\n\nThe difference in absolute numbers will most certainly be within the \nnoise for this one case.\n\n>> no benefit except for some hypothetical future case that's caught by\n>> the test suite anyway -- or by code review.\n>\n> That's not the benefit, the benefit is that the code is clearer.\n\nI don't see that, and I don't like adding a check that I don't expect to \nbe ever needed. Users are safe because the test suite is going to catch \na missing check.\n\nIn general, I think those who introduce dependencies should add the \nnecessary checks. They have to consider the invariants anyway, no \nmatter how many checks you add beforehand for their convenience.\n\n>> I wonder if we already reached the point where we spent more time discussing\n>> this change than the time needed by the envisioned developer to find and fix\n>> the NULL check that suddenly became necessary. :)\n>\n> Maybe, but if what you want is to avoid the discussion, you could just\n> add the extra five characters and be done with it.\n\nOr you could submit a patch on top that adds the check. I'd send it out \nif you'd supply a commit message. My review comment would be \"mostly \nharmless, but I don't like it very much because it's not needed now and \nprobably won't ever\".\n\nBut I'm more interested in a different way forward: Would it make sense \nto push the allocations (and frees) into the merge functions? Currently \nthey duplicate one of the cache entries. Would the merge functions \nbecome too ugly or too big if they'd have to build them themselves, \navoiding duplication? Would it be significantly faster?\n\nRené\n"},{"id":"219236","messageId":"CAMP44s09s26Gyvqu___cQg2LsUnMrzZAa0pbUnGp85fDUEeapg@mail.gmail.com","threadId":"34012","inReplyTo":"51ACBD68.4070506@lsrfire.ath.cx","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-03T16:10:05Z","receivedAt":"2013-06-03T16:10:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Jun 3, 2013 at 10:59 AM, René Scharfe\n wrote:\n> Am 03.06.2013 02:04, schrieb Felipe Contreras:\n>>\n>> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe\n>> wrote:\n>>>\n>>> Am 03.06.2013 01:23, schrieb Felipe Contreras:\n>>>\n>>>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I\n>>>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of\n>>>> 'if (ce != o->df_conflict_entry)'.\n>>>\n>>>\n>>>\n>>> I did assume you meant the latter.\n>>>\n>>>\n>>>> There's no reason not to.\n>>>\n>>>\n>>>\n>>> Only the minor ones already mentioned: More text,\n>>\n>>\n>> Five characters.\n>>\n>>> one more branch in object\n>>> code,\n>>\n>>\n>> Which might actually be more optimal.\n>\n>\n> The difference in absolute numbers will most certainly be within the noise\n> for this one case.\n\nIf you want to ignore the performance, you should ignore the branch as well.\n\n>>> no benefit except for some hypothetical future case that's caught by\n>>> the test suite anyway -- or by code review.\n>>\n>>\n>> That's not the benefit, the benefit is that the code is clearer.\n>\n>\n> I don't see that, and I don't like adding a check that I don't expect to be\n> ever needed.\n\nIt's called self-documenting code; by adding a check for the NULL\npointer, we are stating that ce can be NULL, if we don't do that,\npeople reading that code would need to figure that out themselves.\n\n> Or you could submit a patch on top that adds the check.\n\nI already sent a patch that has that check.\n\nhttp://article.gmane.org/gmane.comp.version-control.git/225972\n\n-- \nFelipe Contreras\n"},{"id":"219244","messageId":"51ACCB14.7030308@lsrfire.ath.cx","threadId":"34012","inReplyTo":"CAMP44s09s26Gyvqu___cQg2LsUnMrzZAa0pbUnGp85fDUEeapg@mail.gmail.com","subject":"[PATCH v2 8/7] unpack-trees: document that pointer ce can be NULL","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-03T16:57:56Z","receivedAt":"2013-06-03T16:57:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"From: Felipe Contreras \n\nIf someone adds code that dereferences ce before it is freed without\nchecking for NULL it will crash sometimes. Spare that person from\nhaving to wonder about the reason.\n\nSigned-off-by: Felipe Contreras \n---\nSignoff from http://article.gmane.org/gmane.comp.version-control.git/225972.\nNo signoff from me because I don't see the point of adding a check for\na developer that probably won't appear.\n\n unpack-trees.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 57b4074..f22bd89 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -605,7 +605,7 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \t\t\t\t\to);\n \t\tfor (i = 0; i < n; i++) {\n \t\t\tstruct cache_entry *ce = src[i + o->merge];\n-\t\t\tif (ce != o->df_conflict_entry)\n+\t\t\tif (ce && ce != o->df_conflict_entry)\n \t\t\t\tfree(ce);\n \t\t}\n \t\treturn rc;\n-- \n1.8.3\n"},{"id":"219256","messageId":"7va9n7gkjq.fsf@alter.siamese.dyndns.org","threadId":"34012","inReplyTo":"CAMP44s09s26Gyvqu___cQg2LsUnMrzZAa0pbUnGp85fDUEeapg@mail.gmail.com","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-03T17:40:09Z","receivedAt":"2013-06-03T17:40:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> I don't see that, and I don't like adding a check that I don't expect to be\n>> ever needed.\n>\n> It's called self-documenting code; by adding a check for the NULL\n> pointer, we are stating that ce can be NULL, if we don't do that,\n> people reading that code would need to figure that out themselves.\n\nPeople following the codepath to unpack_nondirectories() already\nhave seen enough to know what src[] means and very well know what\nNULL in it means. The only people possibly confused are those who\ndo not know free(NULL) is safe, isn't it?\n\nHonestly speaking, I do not want such people to be touching this\npart of the system.\n"},{"id":"219297","messageId":"CAMP44s2b7gNkiVddN8XVm_u4e4WkxWmvS0mB8XLRv-1tn2Qc9g@mail.gmail.com","threadId":"34012","inReplyTo":"7va9n7gkjq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-03T20:53:45Z","receivedAt":"2013-06-03T20:53:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Jun 3, 2013 at 12:40 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>>> I don't see that, and I don't like adding a check that I don't expect to be\n>>> ever needed.\n>>\n>> It's called self-documenting code; by adding a check for the NULL\n>> pointer, we are stating that ce can be NULL, if we don't do that,\n>> people reading that code would need to figure that out themselves.\n>\n> People following the codepath to unpack_nondirectories() already\n> have seen enough to know what src[] means and very well know what\n> NULL in it means. The only people possibly confused are those who\n> do not know free(NULL) is safe, isn't it?\n\nWrong. I still do not know what src[] means, and I don't need to know,\nI can see from the code that the cached entries there leak.\n\n> Honestly speaking, I do not want such people to be touching this\n> part of the system.\n\nSo we should make it more obfuscated?\n\n-- \nFelipe Contreras\n"}]}