[PATCH v2 0/7] unpack-trees: plug memory leak for merges

[PATCH v2 0/7] unpack-trees: plug memory leak for merges

[René Scharfe]

Changes in v2: Only changed the loop style in patch 7, as suggested
by Felipe.

This series adds const to cache_entry pointers in a lot of places, in
order to show that we can free them in unpack_nondirectories, which
the last patch finally does.

First three easy patches for adding const and splitting a function in
two:

  cache: mark cache_entry pointers const
  read-cache: mark cache_entry pointers const
  unpack-trees: factor out dup_entry

Then a patch that reduces the side effects of merged_entry:

  unpack-trees: create working copy of merge entry in merged_entry

Another easy const patch:

  diff-lib, read-tree, unpack-trees: mark cache_entry pointers const

And patch that introduces "const struct cache_entry * const *", which
may look a bit scary at first:

  diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const

Final patch that plugs a memory leak in unpack_nondirectories.

  unpack-trees: free cache_entry array members for merges

It's basically the same one that Stephen tested a while ago
(http://thread.gmane.org/gmane.comp.version-control.git/222887).

It's not the only leak that affects cherry-pick; expect more
(independent) patches.

 builtin/read-tree.c |   5 +-
 cache.h             |  10 ++--
 diff-lib.c          |  26 ++++-----
 read-cache.c        |  18 ++++---
 unpack-trees.c      | 148 ++++++++++++++++++++++++++++++++--------------------
 unpack-trees.h      |  14 +++--
 6 files changed, 133 insertions(+), 88 deletions(-)

-- 
1.8.3

[PATCH v2 1/7] cache: mark cache_entry pointers const

[René Scharfe]

Add const for pointers that are only dereferenced for reading by the
inline functions copy_cache_entry and ce_mode_from_stat.  This allows
callers to pass in const pointers.

Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 cache.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 94ca1ac..43a27e7 100644
--- a/cache.h
+++ b/cache.h
@@ -190,7 +190,8 @@ struct cache_entry {
  * another. But we never change the name, or the hash state!
  */
 #define CE_STATE_MASK (CE_HASHED | CE_UNHASHED)
-static inline void copy_cache_entry(struct cache_entry *dst, struct cache_entry *src)
+static inline void copy_cache_entry(struct cache_entry *dst,
+				    const struct cache_entry *src)
 {
 	unsigned int state = dst->ce_flags & CE_STATE_MASK;
 
@@ -222,7 +223,8 @@ static inline unsigned int create_ce_mode(unsigned int mode)
 		return S_IFGITLINK;
 	return S_IFREG | ce_permissions(mode);
 }
-static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned int mode)
+static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
+					     unsigned int mode)
 {
 	extern int trust_executable_bit, has_symlinks;
 	if (!has_symlinks && S_ISREG(mode) &&
-- 
1.8.3

[PATCH v2 2/7] read-cache: mark cache_entry pointers const

[René Scharfe]

ie_match_stat and ie_modified only derefence their struct cache_entry
pointers for reading.  Add const to the parameter declaration here and
do the same for the static helper function used by them, as it's the
same there as well.  This allows callers to pass in const pointers.

Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 cache.h      |  4 ++--
 read-cache.c | 18 ++++++++++--------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 43a27e7..01e8760 100644
--- a/cache.h
+++ b/cache.h
@@ -482,8 +482,8 @@ extern void *read_blob_data_from_index(struct index_state *, const char *, unsig
 #define CE_MATCH_RACY_IS_DIRTY		02
 /* do stat comparison even if CE_SKIP_WORKTREE is true */
 #define CE_MATCH_IGNORE_SKIP_WORKTREE	04
-extern int ie_match_stat(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
-extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
+extern int ie_match_stat(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
+extern int ie_modified(const struct index_state *, const struct cache_entry *, struct stat *, unsigned int);
 
 #define PATHSPEC_ONESTAR 1	/* the pathspec pattern sastisfies GFNM_ONESTAR */
 
diff --git a/read-cache.c b/read-cache.c
index 04ed561..e6e0466 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -91,7 +91,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
 		ce_mark_uptodate(ce);
 }
 
-static int ce_compare_data(struct cache_entry *ce, struct stat *st)
+static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 {
 	int match = -1;
 	int fd = open(ce->name, O_RDONLY);
@@ -105,7 +105,7 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st)
 	return match;
 }
 
-static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
+static int ce_compare_link(const struct cache_entry *ce, size_t expected_size)
 {
 	int match = -1;
 	void *buffer;
@@ -126,7 +126,7 @@ static int ce_compare_link(struct cache_entry *ce, size_t expected_size)
 	return match;
 }
 
-static int ce_compare_gitlink(struct cache_entry *ce)
+static int ce_compare_gitlink(const struct cache_entry *ce)
 {
 	unsigned char sha1[20];
 
@@ -143,7 +143,7 @@ static int ce_compare_gitlink(struct cache_entry *ce)
 	return hashcmp(sha1, ce->sha1);
 }
 
-static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
+static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)
 {
 	switch (st->st_mode & S_IFMT) {
 	case S_IFREG:
@@ -163,7 +163,7 @@ static int ce_modified_check_fs(struct cache_entry *ce, struct stat *st)
 	return 0;
 }
 
-static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
+static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
 {
 	unsigned int changed = 0;
 
@@ -239,7 +239,8 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
 	return changed;
 }
 
-static int is_racy_timestamp(const struct index_state *istate, struct cache_entry *ce)
+static int is_racy_timestamp(const struct index_state *istate,
+			     const struct cache_entry *ce)
 {
 	return (!S_ISGITLINK(ce->ce_mode) &&
 		istate->timestamp.sec &&
@@ -255,7 +256,7 @@ static int is_racy_timestamp(const struct index_state *istate, struct cache_entr
 }
 
 int ie_match_stat(const struct index_state *istate,
-		  struct cache_entry *ce, struct stat *st,
+		  const struct cache_entry *ce, struct stat *st,
 		  unsigned int options)
 {
 	unsigned int changed;
@@ -311,7 +312,8 @@ int ie_match_stat(const struct index_state *istate,
 }
 
 int ie_modified(const struct index_state *istate,
-		struct cache_entry *ce, struct stat *st, unsigned int options)
+		const struct cache_entry *ce,
+		struct stat *st, unsigned int options)
 {
 	int changed, changed_fs;
 
-- 
1.8.3

[PATCH v2 3/7] unpack-trees: factor out dup_entry

[René Scharfe]

While we're add it, mark the struct cache_entry pointer of add_entry
const because we only read from it and this allows callers to pass in
const pointers.

Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 unpack-trees.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index ede4299..e8b4cc1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -116,14 +116,20 @@ static void do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 			ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
 }
 
-static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
-	unsigned int set, unsigned int clear)
+static struct cache_entry *dup_entry(const struct cache_entry *ce)
 {
 	unsigned int size = ce_size(ce);
 	struct cache_entry *new = xmalloc(size);
 
 	memcpy(new, ce, size);
-	do_add_entry(o, new, set, clear);
+	return new;
+}
+
+static void add_entry(struct unpack_trees_options *o,
+		      const struct cache_entry *ce,
+		      unsigned int set, unsigned int clear)
+{
+	do_add_entry(o, dup_entry(ce), set, clear);
 }
 
 /*
-- 
1.8.3

[PATCH v2 4/7] unpack-trees: create working copy of merge entry in merged_entry

[René Scharfe]

Duplicate the merge entry right away and work with that instead of
modifying the entry we got and duplicating it only at the end of
the function.  Then mark that pointer const to document that we
don't modify the referenced cache_entry.

This change is safe because all existing merge functions call
merged_entry just before returning (or not at all), i.e. they don't
care about changes to the referenced cache_entry after the call.
unpack_nondirectories and unpack_index_entry, which call the merge
functions through call_unpack_fn, aren't interested in such changes
neither.

The change complicates merged_entry a bit because we have to free the
copy if we error out, but allows callers to pass a const pointer.

Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 unpack-trees.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index e8b4cc1..2fecef8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1466,10 +1466,12 @@ static int verify_absent_sparse(struct cache_entry *ce,
 	return verify_absent_1(ce, orphaned_error, o);
 }
 
-static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
-		struct unpack_trees_options *o)
+static int merged_entry(const struct cache_entry *ce,
+			struct cache_entry *old,
+			struct unpack_trees_options *o)
 {
 	int update = CE_UPDATE;
+	struct cache_entry *merge = dup_entry(ce);
 
 	if (!old) {
 		/*
@@ -1487,8 +1489,11 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		update |= CE_ADDED;
 		merge->ce_flags |= CE_NEW_SKIP_WORKTREE;
 
-		if (verify_absent(merge, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o))
+		if (verify_absent(merge,
+				  ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o)) {
+			free(merge);
 			return -1;
+		}
 		invalidate_ce_path(merge, o);
 	} else if (!(old->ce_flags & CE_CONFLICTED)) {
 		/*
@@ -1502,8 +1507,10 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 			copy_cache_entry(merge, old);
 			update = 0;
 		} else {
-			if (verify_uptodate(old, o))
+			if (verify_uptodate(old, o)) {
+				free(merge);
 				return -1;
+			}
 			/* Migrate old flags over */
 			update |= old->ce_flags & (CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE);
 			invalidate_ce_path(old, o);
@@ -1516,7 +1523,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 		invalidate_ce_path(old, o);
 	}
 
-	add_entry(o, merge, update, CE_STAGEMASK);
+	do_add_entry(o, merge, update, CE_STAGEMASK);
 	return 1;
 }
 
-- 
1.8.3

[PATCH v2 5/7] diff-lib, read-tree, unpack-trees: mark cache_entry pointers const

[René Scharfe]

Add const to struct cache_entry pointers throughout the tree which are
only used for reading.  This allows callers to pass in const pointers.

Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/read-tree.c |  2 +-
 diff-lib.c          | 23 +++++++-------
 unpack-trees.c      | 91 +++++++++++++++++++++++++++++------------------------
 3 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 042ac1b..b847486 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -66,7 +66,7 @@ static int exclude_per_directory_cb(const struct option *opt, const char *arg,
 	return 0;
 }
 
-static void debug_stage(const char *label, struct cache_entry *ce,
+static void debug_stage(const char *label, const struct cache_entry *ce,
 			struct unpack_trees_options *o)
 {
 	printf("%s ", label);
diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..83d0cb8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -64,8 +64,9 @@ static int check_removed(const struct cache_entry *ce, struct stat *st)
  * commits, untracked content and/or modified content).
  */
 static int match_stat_with_submodule(struct diff_options *diffopt,
-				      struct cache_entry *ce, struct stat *st,
-				      unsigned ce_option, unsigned *dirty_submodule)
+				     const struct cache_entry *ce,
+				     struct stat *st, unsigned ce_option,
+				     unsigned *dirty_submodule)
 {
 	int changed = ce_match_stat(ce, st, ce_option);
 	if (S_ISGITLINK(ce->ce_mode)) {
@@ -237,7 +238,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 /* A file entry went away or appeared */
 static void diff_index_show_file(struct rev_info *revs,
 				 const char *prefix,
-				 struct cache_entry *ce,
+				 const struct cache_entry *ce,
 				 const unsigned char *sha1, int sha1_valid,
 				 unsigned int mode,
 				 unsigned dirty_submodule)
@@ -246,7 +247,7 @@ static void diff_index_show_file(struct rev_info *revs,
 		       sha1, sha1_valid, ce->name, dirty_submodule);
 }
 
-static int get_stat_data(struct cache_entry *ce,
+static int get_stat_data(const struct cache_entry *ce,
 			 const unsigned char **sha1p,
 			 unsigned int *modep,
 			 int cached, int match_missing,
@@ -283,7 +284,7 @@ static int get_stat_data(struct cache_entry *ce,
 }
 
 static void show_new_file(struct rev_info *revs,
-			  struct cache_entry *new,
+			  const struct cache_entry *new,
 			  int cached, int match_missing)
 {
 	const unsigned char *sha1;
@@ -302,8 +303,8 @@ static void show_new_file(struct rev_info *revs,
 }
 
 static int show_modified(struct rev_info *revs,
-			 struct cache_entry *old,
-			 struct cache_entry *new,
+			 const struct cache_entry *old,
+			 const struct cache_entry *new,
 			 int report_missing,
 			 int cached, int match_missing)
 {
@@ -362,8 +363,8 @@ static int show_modified(struct rev_info *revs,
  * give you the position and number of entries in the index).
  */
 static void do_oneway_diff(struct unpack_trees_options *o,
-	struct cache_entry *idx,
-	struct cache_entry *tree)
+			   const struct cache_entry *idx,
+			   const struct cache_entry *tree)
 {
 	struct rev_info *revs = o->unpack_data;
 	int match_missing, cached;
@@ -425,8 +426,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
  */
 static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)
 {
-	struct cache_entry *idx = src[0];
-	struct cache_entry *tree = src[1];
+	const struct cache_entry *idx = src[0];
+	const struct cache_entry *tree = src[1];
 	struct rev_info *revs = o->unpack_data;
 
 	/*
diff --git a/unpack-trees.c b/unpack-trees.c
index 2fecef8..c5a40df 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -241,8 +241,11 @@ static int check_updates(struct unpack_trees_options *o)
 	return errs != 0;
 }
 
-static int verify_uptodate_sparse(struct cache_entry *ce, struct unpack_trees_options *o);
-static int verify_absent_sparse(struct cache_entry *ce, enum unpack_trees_error_types, struct unpack_trees_options *o);
+static int verify_uptodate_sparse(const struct cache_entry *ce,
+				  struct unpack_trees_options *o);
+static int verify_absent_sparse(const struct cache_entry *ce,
+				enum unpack_trees_error_types,
+				struct unpack_trees_options *o);
 
 static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_options *o)
 {
@@ -326,7 +329,7 @@ static void mark_all_ce_unused(struct index_state *index)
 		index->cache[i]->ce_flags &= ~(CE_UNPACKED | CE_ADDED | CE_NEW_SKIP_WORKTREE);
 }
 
-static int locate_in_src_index(struct cache_entry *ce,
+static int locate_in_src_index(const struct cache_entry *ce,
 			       struct unpack_trees_options *o)
 {
 	struct index_state *index = o->src_index;
@@ -1001,7 +1004,9 @@ static void mark_new_skip_worktree(struct exclude_list *el,
 		       select_flag, skip_wt_flag, el);
 }
 
-static int verify_absent(struct cache_entry *, enum unpack_trees_error_types, struct unpack_trees_options *);
+static int verify_absent(const struct cache_entry *,
+			 enum unpack_trees_error_types,
+			 struct unpack_trees_options *);
 /*
  * N-way merge "len" trees.  Returns 0 on success, -1 on failure to manipulate the
  * resulting index, -2 on failure to reflect the changes to the work tree.
@@ -1171,12 +1176,13 @@ return_failed:
 
 /* Here come the merge functions */
 
-static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)
+static int reject_merge(const struct cache_entry *ce,
+			struct unpack_trees_options *o)
 {
 	return add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce->name);
 }
 
-static int same(struct cache_entry *a, struct cache_entry *b)
+static int same(const struct cache_entry *a, const struct cache_entry *b)
 {
 	if (!!a != !!b)
 		return 0;
@@ -1193,9 +1199,9 @@ static int same(struct cache_entry *a, struct cache_entry *b)
  * When a CE gets turned into an unmerged entry, we
  * want it to be up-to-date
  */
-static int verify_uptodate_1(struct cache_entry *ce,
-				   struct unpack_trees_options *o,
-				   enum unpack_trees_error_types error_type)
+static int verify_uptodate_1(const struct cache_entry *ce,
+			     struct unpack_trees_options *o,
+			     enum unpack_trees_error_types error_type)
 {
 	struct stat st;
 
@@ -1234,7 +1240,7 @@ static int verify_uptodate_1(struct cache_entry *ce,
 		add_rejected_path(o, error_type, ce->name);
 }
 
-static int verify_uptodate(struct cache_entry *ce,
+static int verify_uptodate(const struct cache_entry *ce,
 			   struct unpack_trees_options *o)
 {
 	if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))
@@ -1242,13 +1248,14 @@ static int verify_uptodate(struct cache_entry *ce,
 	return verify_uptodate_1(ce, o, ERROR_NOT_UPTODATE_FILE);
 }
 
-static int verify_uptodate_sparse(struct cache_entry *ce,
+static int verify_uptodate_sparse(const struct cache_entry *ce,
 				  struct unpack_trees_options *o)
 {
 	return verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE);
 }
 
-static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_options *o)
+static void invalidate_ce_path(const struct cache_entry *ce,
+			       struct unpack_trees_options *o)
 {
 	if (ce)
 		cache_tree_invalidate_path(o->src_index->cache_tree, ce->name);
@@ -1261,16 +1268,16 @@ static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_optio
  * Currently, git does not checkout subprojects during a superproject
  * checkout, so it is not going to overwrite anything.
  */
-static int verify_clean_submodule(struct cache_entry *ce,
-				      enum unpack_trees_error_types error_type,
-				      struct unpack_trees_options *o)
+static int verify_clean_submodule(const struct cache_entry *ce,
+				  enum unpack_trees_error_types error_type,
+				  struct unpack_trees_options *o)
 {
 	return 0;
 }
 
-static int verify_clean_subdirectory(struct cache_entry *ce,
-				      enum unpack_trees_error_types error_type,
-				      struct unpack_trees_options *o)
+static int verify_clean_subdirectory(const struct cache_entry *ce,
+				     enum unpack_trees_error_types error_type,
+				     struct unpack_trees_options *o)
 {
 	/*
 	 * we are about to extract "ce->name"; we would not want to lose
@@ -1356,7 +1363,7 @@ static int icase_exists(struct unpack_trees_options *o, const char *name, int le
 }
 
 static int check_ok_to_remove(const char *name, int len, int dtype,
-			      struct cache_entry *ce, struct stat *st,
+			      const struct cache_entry *ce, struct stat *st,
 			      enum unpack_trees_error_types error_type,
 			      struct unpack_trees_options *o)
 {
@@ -1411,9 +1418,9 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
  * We do not want to remove or overwrite a working tree file that
  * is not tracked, unless it is ignored.
  */
-static int verify_absent_1(struct cache_entry *ce,
-				 enum unpack_trees_error_types error_type,
-				 struct unpack_trees_options *o)
+static int verify_absent_1(const struct cache_entry *ce,
+			   enum unpack_trees_error_types error_type,
+			   struct unpack_trees_options *o)
 {
 	int len;
 	struct stat st;
@@ -1446,7 +1453,7 @@ static int verify_absent_1(struct cache_entry *ce,
 	}
 }
 
-static int verify_absent(struct cache_entry *ce,
+static int verify_absent(const struct cache_entry *ce,
 			 enum unpack_trees_error_types error_type,
 			 struct unpack_trees_options *o)
 {
@@ -1455,9 +1462,9 @@ static int verify_absent(struct cache_entry *ce,
 	return verify_absent_1(ce, error_type, o);
 }
 
-static int verify_absent_sparse(struct cache_entry *ce,
-			 enum unpack_trees_error_types error_type,
-			 struct unpack_trees_options *o)
+static int verify_absent_sparse(const struct cache_entry *ce,
+				enum unpack_trees_error_types error_type,
+				struct unpack_trees_options *o)
 {
 	enum unpack_trees_error_types orphaned_error = error_type;
 	if (orphaned_error == ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN)
@@ -1467,7 +1474,7 @@ static int verify_absent_sparse(struct cache_entry *ce,
 }
 
 static int merged_entry(const struct cache_entry *ce,
-			struct cache_entry *old,
+			const struct cache_entry *old,
 			struct unpack_trees_options *o)
 {
 	int update = CE_UPDATE;
@@ -1527,8 +1534,9 @@ static int merged_entry(const struct cache_entry *ce,
 	return 1;
 }
 
-static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
-		struct unpack_trees_options *o)
+static int deleted_entry(const struct cache_entry *ce,
+			 const struct cache_entry *old,
+			 struct unpack_trees_options *o)
 {
 	/* Did it exist in the index? */
 	if (!old) {
@@ -1543,7 +1551,8 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
 	return 1;
 }
 
-static int keep_entry(struct cache_entry *ce, struct unpack_trees_options *o)
+static int keep_entry(const struct cache_entry *ce,
+		      struct unpack_trees_options *o)
 {
 	add_entry(o, ce, 0, 0);
 	return 1;
@@ -1567,9 +1576,9 @@ static void show_stage_entry(FILE *o,
 
 int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 {
-	struct cache_entry *index;
-	struct cache_entry *head;
-	struct cache_entry *remote = stages[o->head_idx + 1];
+	const struct cache_entry *index;
+	const struct cache_entry *head;
+	const struct cache_entry *remote = stages[o->head_idx + 1];
 	int count;
 	int head_match = 0;
 	int remote_match = 0;
@@ -1654,7 +1663,7 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 	if (o->aggressive) {
 		int head_deleted = !head;
 		int remote_deleted = !remote;
-		struct cache_entry *ce = NULL;
+		const struct cache_entry *ce = NULL;
 
 		if (index)
 			ce = index;
@@ -1739,9 +1748,9 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
  */
 int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
 {
-	struct cache_entry *current = src[0];
-	struct cache_entry *oldtree = src[1];
-	struct cache_entry *newtree = src[2];
+	const struct cache_entry *current = src[0];
+	const struct cache_entry *oldtree = src[1];
+	const struct cache_entry *newtree = src[2];
 
 	if (o->merge_size != 2)
 		return error("Cannot do a twoway merge of %d trees",
@@ -1806,8 +1815,8 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
 int bind_merge(struct cache_entry **src,
 		struct unpack_trees_options *o)
 {
-	struct cache_entry *old = src[0];
-	struct cache_entry *a = src[1];
+	const struct cache_entry *old = src[0];
+	const struct cache_entry *a = src[1];
 
 	if (o->merge_size != 1)
 		return error("Cannot do a bind merge of %d trees",
@@ -1829,8 +1838,8 @@ int bind_merge(struct cache_entry **src,
  */
 int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)
 {
-	struct cache_entry *old = src[0];
-	struct cache_entry *a = src[1];
+	const struct cache_entry *old = src[0];
+	const struct cache_entry *a = src[1];
 
 	if (o->merge_size != 1)
 		return error("Cannot do a oneway merge of %d trees",
-- 
1.8.3

[PATCH v2 6/7] diff-lib, read-tree, unpack-trees: mark cache_entry array paramters const

[René Scharfe]

Change the type merge_fn_t to accept the array of cache_entry pointers
as const pointers to const pointers.  This documents the fact that the
merge functions don't modify the cache_entry contents or replace any of
the pointers in the array.

Only a single cast is necessary in unpack_nondirectories because adding
two const modifiers at once is not allowed in C.  The cast is safe in
that it doesn't mask any modfication; call_unpack_fn only needs the
array for reading.

Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/read-tree.c |  3 ++-
 diff-lib.c          |  3 ++-
 unpack-trees.c      | 21 +++++++++++++--------
 unpack-trees.h      | 14 +++++++++-----
 4 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index b847486..0f5d7fe 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -80,7 +80,8 @@ static void debug_stage(const char *label, const struct cache_entry *ce,
 		       sha1_to_hex(ce->sha1));
 }
 
-static int debug_merge(struct cache_entry **stages, struct unpack_trees_options *o)
+static int debug_merge(const struct cache_entry * const *stages,
+		       struct unpack_trees_options *o)
 {
 	int i;
 
diff --git a/diff-lib.c b/diff-lib.c
index 83d0cb8..b6f4b21 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -424,7 +424,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
  * the fairly complex unpack_trees() semantic requirements, including
  * the skipping, the path matching, the type conflict cases etc.
  */
-static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)
+static int oneway_diff(const struct cache_entry * const *src,
+		       struct unpack_trees_options *o)
 {
 	const struct cache_entry *idx = src[0];
 	const struct cache_entry *tree = src[1];
diff --git a/unpack-trees.c b/unpack-trees.c
index c5a40df..2dbc05d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -300,7 +300,8 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 	return 0;
 }
 
-static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_options *o)
+static inline int call_unpack_fn(const struct cache_entry * const *src,
+				 struct unpack_trees_options *o)
 {
 	int ret = o->fn(src, o);
 	if (ret > 0)
@@ -397,7 +398,7 @@ static void add_same_unmerged(struct cache_entry *ce,
 static int unpack_index_entry(struct cache_entry *ce,
 			      struct unpack_trees_options *o)
 {
-	struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
+	const struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
 	int ret;
 
 	src[0] = ce;
@@ -600,7 +601,8 @@ static int unpack_nondirectories(int n, unsigned long mask,
 	}
 
 	if (o->merge)
-		return call_unpack_fn(src, o);
+		return call_unpack_fn((const struct cache_entry * const *)src,
+				      o);
 
 	for (i = 0; i < n; i++)
 		if (src[i] && src[i] != o->df_conflict_entry)
@@ -1574,7 +1576,8 @@ static void show_stage_entry(FILE *o,
 }
 #endif
 
-int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
+int threeway_merge(const struct cache_entry * const *stages,
+		   struct unpack_trees_options *o)
 {
 	const struct cache_entry *index;
 	const struct cache_entry *head;
@@ -1746,7 +1749,8 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
  * "carry forward" rule, please see <Documentation/git-read-tree.txt>.
  *
  */
-int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
+int twoway_merge(const struct cache_entry * const *src,
+		 struct unpack_trees_options *o)
 {
 	const struct cache_entry *current = src[0];
 	const struct cache_entry *oldtree = src[1];
@@ -1812,8 +1816,8 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
  * Keep the index entries at stage0, collapse stage1 but make sure
  * stage0 does not have anything there.
  */
-int bind_merge(struct cache_entry **src,
-		struct unpack_trees_options *o)
+int bind_merge(const struct cache_entry * const *src,
+	       struct unpack_trees_options *o)
 {
 	const struct cache_entry *old = src[0];
 	const struct cache_entry *a = src[1];
@@ -1836,7 +1840,8 @@ int bind_merge(struct cache_entry **src,
  * The rule is:
  * - take the stat information from stage0, take the data from stage1
  */
-int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)
+int oneway_merge(const struct cache_entry * const *src,
+		 struct unpack_trees_options *o)
 {
 	const struct cache_entry *old = src[0];
 	const struct cache_entry *a = src[1];
diff --git a/unpack-trees.h b/unpack-trees.h
index 5e432f5..36a73a6 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -8,7 +8,7 @@
 struct unpack_trees_options;
 struct exclude_list;
 
-typedef int (*merge_fn_t)(struct cache_entry **src,
+typedef int (*merge_fn_t)(const struct cache_entry * const *src,
 		struct unpack_trees_options *options);
 
 enum unpack_trees_error_types {
@@ -77,9 +77,13 @@ struct unpack_trees_options {
 extern int unpack_trees(unsigned n, struct tree_desc *t,
 		struct unpack_trees_options *options);
 
-int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o);
-int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o);
-int bind_merge(struct cache_entry **src, struct unpack_trees_options *o);
-int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o);
+int threeway_merge(const struct cache_entry * const *stages,
+		   struct unpack_trees_options *o);
+int twoway_merge(const struct cache_entry * const *src,
+		 struct unpack_trees_options *o);
+int bind_merge(const struct cache_entry * const *src,
+	       struct unpack_trees_options *o);
+int oneway_merge(const struct cache_entry * const *src,
+		 struct unpack_trees_options *o);
 
 #endif
-- 
1.8.3

[PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[René Scharfe]

The merge functions duplicate entries as needed and they don't free
them.  Release them in unpack_nondirectories, the same function
where they were allocated, after we're done.

As suggested by Felipe, use the same loop style (zero-based for loop)
for freeing as for allocating.

Improved-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 unpack-trees.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 2dbc05d..57b4074 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -600,9 +600,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
 		src[i + o->merge] = create_ce_entry(info, names + i, stage);
 	}
 
-	if (o->merge)
-		return call_unpack_fn((const struct cache_entry * const *)src,
-				      o);
+	if (o->merge) {
+		int rc = call_unpack_fn((const struct cache_entry * const *)src,
+					o);
+		for (i = 0; i < n; i++) {
+			struct cache_entry *ce = src[i + o->merge];
+			if (ce != o->df_conflict_entry)
+				free(ce);
+		}
+		return rc;
+	}
 
 	for (i = 0; i < n; i++)
 		if (src[i] && src[i] != o->df_conflict_entry)
-- 
1.8.3

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Felipe Contreras]

On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> The merge functions duplicate entries as needed and they don't free
> them.  Release them in unpack_nondirectories, the same function
> where they were allocated, after we're done.
>
> As suggested by Felipe, use the same loop style (zero-based for loop)
> for freeing as for allocating.
>
> Improved-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>  unpack-trees.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 2dbc05d..57b4074 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -600,9 +600,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
>                 src[i + o->merge] = create_ce_entry(info, names + i, stage);
>         }
>
> -       if (o->merge)
> -               return call_unpack_fn((const struct cache_entry * const *)src,
> -                                     o);
> +       if (o->merge) {
> +               int rc = call_unpack_fn((const struct cache_entry * const *)src,
> +                                       o);
> +               for (i = 0; i < n; i++) {
> +                       struct cache_entry *ce = src[i + o->merge];
> +                       if (ce != o->df_conflict_entry)

It's possible that ce is NULL, but you didn't add that check because
free(NULL) still works? Or because ce cannot be NULL?

If it's the former, I think it's clearer if we check that ce is not
NULL either way.

Otherwise it's OK by me.

> +                               free(ce);
> +               }
> +               return rc;
> +       }

-- 
Felipe Contreras

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[René Scharfe]

Am 02.06.2013 19:25, schrieb Felipe Contreras:
> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> +               for (i = 0; i < n; i++) {
>> +                       struct cache_entry *ce = src[i + o->merge];
>> +                       if (ce != o->df_conflict_entry)
>
> It's possible that ce is NULL, but you didn't add that check because
> free(NULL) still works? Or because ce cannot be NULL?
>
> If it's the former, I think it's clearer if we check that ce is not
> NULL either way.

It is NULL if one tree misses an entry (e.g. a new or removed file). 
free handles NULL and we generally avoid duplicating its NULL-check.

You're probably referring to the non-merge case as the example for 
checking.  That one is different, though, because there we call 
do_add_entry instead of free, which does not handle NULL.  And it 
doesn't have to, as it is mostly called through add_entry, which never 
passes NULL.

René

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Felipe Contreras]

On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 02.06.2013 19:25, schrieb Felipe Contreras:
>>
>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>
>>> +               for (i = 0; i < n; i++) {
>>> +                       struct cache_entry *ce = src[i + o->merge];
>>> +                       if (ce != o->df_conflict_entry)
>>
>>
>> It's possible that ce is NULL, but you didn't add that check because
>> free(NULL) still works? Or because ce cannot be NULL?
>>
>> If it's the former, I think it's clearer if we check that ce is not
>> NULL either way.
>
>
> It is NULL if one tree misses an entry (e.g. a new or removed file). free
> handles NULL and we generally avoid duplicating its NULL-check.

Yeah, but I can see somebody adding code inside that 'if' clause to
print the cache entry, and see a crash only to wonder what's going on.
And to save what? 5 characters?

-- 
Felipe Contreras

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[René Scharfe]

Am 02.06.2013 19:59, schrieb Felipe Contreras:
> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 02.06.2013 19:25, schrieb Felipe Contreras:
>>>
>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>
>>>> +               for (i = 0; i < n; i++) {
>>>> +                       struct cache_entry *ce = src[i + o->merge];
>>>> +                       if (ce != o->df_conflict_entry)
>>>
>>>
>>> It's possible that ce is NULL, but you didn't add that check because
>>> free(NULL) still works? Or because ce cannot be NULL?
>>>
>>> If it's the former, I think it's clearer if we check that ce is not
>>> NULL either way.
>>
>>
>> It is NULL if one tree misses an entry (e.g. a new or removed file). free
>> handles NULL and we generally avoid duplicating its NULL-check.
>
> Yeah, but I can see somebody adding code inside that 'if' clause to
> print the cache entry, and see a crash only to wonder what's going on.
> And to save what? 5 characters?

The person adding code that depends on ce not being NULL needs to add 
that check as well.  Let's not worry too much about future changes that 
may or (more likely IMHO) may not be done.  The test suite covers this 
case multiple times, so such a mistake doesn't have a realistic chance 
to hit master.

René

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Felipe Contreras]

On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 02.06.2013 19:59, schrieb Felipe Contreras:
>
>> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>
>>> Am 02.06.2013 19:25, schrieb Felipe Contreras:
>>>>
>>>>
>>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
>>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>>
>>>>>
>>>>> +               for (i = 0; i < n; i++) {
>>>>> +                       struct cache_entry *ce = src[i + o->merge];
>>>>> +                       if (ce != o->df_conflict_entry)
>>>>
>>>>
>>>>
>>>> It's possible that ce is NULL, but you didn't add that check because
>>>> free(NULL) still works? Or because ce cannot be NULL?
>>>>
>>>> If it's the former, I think it's clearer if we check that ce is not
>>>> NULL either way.
>>>
>>>
>>>
>>> It is NULL if one tree misses an entry (e.g. a new or removed file). free
>>> handles NULL and we generally avoid duplicating its NULL-check.
>>
>>
>> Yeah, but I can see somebody adding code inside that 'if' clause to
>> print the cache entry, and see a crash only to wonder what's going on.
>> And to save what? 5 characters?
>
>
> The person adding code that depends on ce not being NULL needs to add that
> check as well.  Let's not worry too much about future changes that may or
> (more likely IMHO) may not be done.  The test suite covers this case
> multiple times, so such a mistake doesn't have a realistic chance to hit
> master.

What do we gain by not doing this? 5 less characters?

-- 
Felipe Contreras

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[René Scharfe]

Am 03.06.2013 00:38, schrieb Felipe Contreras:
> On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 02.06.2013 19:59, schrieb Felipe Contreras:
>>
>>> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>
>>>> Am 02.06.2013 19:25, schrieb Felipe Contreras:
>>>>>
>>>>>
>>>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
>>>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>>>
>>>>>>
>>>>>> +               for (i = 0; i < n; i++) {
>>>>>> +                       struct cache_entry *ce = src[i + o->merge];
>>>>>> +                       if (ce != o->df_conflict_entry)
>>>>>
>>>>>
>>>>>
>>>>> It's possible that ce is NULL, but you didn't add that check because
>>>>> free(NULL) still works? Or because ce cannot be NULL?
>>>>>
>>>>> If it's the former, I think it's clearer if we check that ce is not
>>>>> NULL either way.
>>>>
>>>>
>>>>
>>>> It is NULL if one tree misses an entry (e.g. a new or removed file). free
>>>> handles NULL and we generally avoid duplicating its NULL-check.
>>>
>>>
>>> Yeah, but I can see somebody adding code inside that 'if' clause to
>>> print the cache entry, and see a crash only to wonder what's going on.
>>> And to save what? 5 characters?
>>
>>
>> The person adding code that depends on ce not being NULL needs to add that
>> check as well.  Let's not worry too much about future changes that may or
>> (more likely IMHO) may not be done.  The test suite covers this case
>> multiple times, so such a mistake doesn't have a realistic chance to hit
>> master.
>
> What do we gain by not doing this? 5 less characters?

By following the convention of not checking for NULL when freeing, we 
reduce the size of the code slightly and have one less branch 
instruction in the object code.  That's not particularly exciting in a 
single instance but makes a difference if followed throughout the code base.

What do we gain by adding a duplicate check?  A few minutes of debugging 
time by the person who will add some code and forget the NULL check? 
And how likely is that to happen?

René

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Felipe Contreras]

On Sun, Jun 2, 2013 at 6:06 PM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 03.06.2013 00:38, schrieb Felipe Contreras:
>
>> On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe
>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>
>>> Am 02.06.2013 19:59, schrieb Felipe Contreras:
>>>
>>>> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe
>>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>>
>>>>>
>>>>> Am 02.06.2013 19:25, schrieb Felipe Contreras:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe
>>>>>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> +               for (i = 0; i < n; i++) {
>>>>>>> +                       struct cache_entry *ce = src[i + o->merge];
>>>>>>> +                       if (ce != o->df_conflict_entry)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> It's possible that ce is NULL, but you didn't add that check because
>>>>>> free(NULL) still works? Or because ce cannot be NULL?
>>>>>>
>>>>>> If it's the former, I think it's clearer if we check that ce is not
>>>>>> NULL either way.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> It is NULL if one tree misses an entry (e.g. a new or removed file).
>>>>> free
>>>>> handles NULL and we generally avoid duplicating its NULL-check.
>>>>
>>>>
>>>>
>>>> Yeah, but I can see somebody adding code inside that 'if' clause to
>>>> print the cache entry, and see a crash only to wonder what's going on.
>>>> And to save what? 5 characters?
>>>
>>>
>>>
>>> The person adding code that depends on ce not being NULL needs to add
>>> that
>>> check as well.  Let's not worry too much about future changes that may or
>>> (more likely IMHO) may not be done.  The test suite covers this case
>>> multiple times, so such a mistake doesn't have a realistic chance to hit
>>> master.
>>
>>
>> What do we gain by not doing this? 5 less characters?
>
>
> By following the convention of not checking for NULL when freeing,

That's not what I asked.

I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
'if (ce != o->df_conflict_entry)'.

There's no reason not to.

-- 
Felipe Contreras

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[René Scharfe]

Am 03.06.2013 01:23, schrieb Felipe Contreras:
> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
> 'if (ce != o->df_conflict_entry)'.

I did assume you meant the latter.

> There's no reason not to.

Only the minor ones already mentioned: More text, one more branch in 
object code, no benefit except for some hypothetical future case that's 
caught by the test suite anyway -- or by code review.

I wonder if we already reached the point where we spent more time 
discussing this change than the time needed by the envisioned developer 
to find and fix the NULL check that suddenly became necessary. :)

René

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Felipe Contreras]

On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 03.06.2013 01:23, schrieb Felipe Contreras:
>
>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
>> 'if (ce != o->df_conflict_entry)'.
>
>
> I did assume you meant the latter.
>
>
>> There's no reason not to.
>
>
> Only the minor ones already mentioned: More text,

Five characters.

> one more branch in object
> code,

Which might actually be more optimal.

> no benefit except for some hypothetical future case that's caught by
> the test suite anyway -- or by code review.

That's not the benefit, the benefit is that the code is clearer.

> I wonder if we already reached the point where we spent more time discussing
> this change than the time needed by the envisioned developer to find and fix
> the NULL check that suddenly became necessary. :)

Maybe, but if what you want is to avoid the discussion, you could just
add the extra five characters and be done with it.

-- 
Felipe Contreras

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[René Scharfe]

Am 03.06.2013 02:04, schrieb Felipe Contreras:
> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 03.06.2013 01:23, schrieb Felipe Contreras:
>>
>>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
>>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
>>> 'if (ce != o->df_conflict_entry)'.
>>
>>
>> I did assume you meant the latter.
>>
>>
>>> There's no reason not to.
>>
>>
>> Only the minor ones already mentioned: More text,
>
> Five characters.
>
>> one more branch in object
>> code,
>
> Which might actually be more optimal.

The difference in absolute numbers will most certainly be within the 
noise for this one case.

>> no benefit except for some hypothetical future case that's caught by
>> the test suite anyway -- or by code review.
>
> That's not the benefit, the benefit is that the code is clearer.

I don't see that, and I don't like adding a check that I don't expect to 
be ever needed.  Users are safe because the test suite is going to catch 
a missing check.

In general, I think those who introduce dependencies should add the 
necessary checks.  They have to consider the invariants anyway, no 
matter how many checks you add beforehand for their convenience.

>> I wonder if we already reached the point where we spent more time discussing
>> this change than the time needed by the envisioned developer to find and fix
>> the NULL check that suddenly became necessary. :)
>
> Maybe, but if what you want is to avoid the discussion, you could just
> add the extra five characters and be done with it.

Or you could submit a patch on top that adds the check.  I'd send it out 
if you'd supply a commit message.  My review comment would be "mostly 
harmless, but I don't like it very much because it's not needed now and 
probably won't ever".

But I'm more interested in a different way forward: Would it make sense 
to push the allocations (and frees) into the merge functions?  Currently 
they duplicate one of the cache entries.  Would the merge functions 
become too ugly or too big if they'd have to build them themselves, 
avoiding duplication?  Would it be significantly faster?

René

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Felipe Contreras]

On Mon, Jun 3, 2013 at 10:59 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 03.06.2013 02:04, schrieb Felipe Contreras:
>>
>> On Sun, Jun 2, 2013 at 6:47 PM, René Scharfe
>> <rene.scharfe@lsrfire.ath.cx> wrote:
>>>
>>> Am 03.06.2013 01:23, schrieb Felipe Contreras:
>>>
>>>> I didn't say we should do 'if (ce) free(ce);' instead of 'free(ce);' I
>>>> said we should do 'if (cd && ce != o->df_conflict_entry)' instead of
>>>> 'if (ce != o->df_conflict_entry)'.
>>>
>>>
>>>
>>> I did assume you meant the latter.
>>>
>>>
>>>> There's no reason not to.
>>>
>>>
>>>
>>> Only the minor ones already mentioned: More text,
>>
>>
>> Five characters.
>>
>>> one more branch in object
>>> code,
>>
>>
>> Which might actually be more optimal.
>
>
> The difference in absolute numbers will most certainly be within the noise
> for this one case.

If you want to ignore the performance, you should ignore the branch as well.

>>> no benefit except for some hypothetical future case that's caught by
>>> the test suite anyway -- or by code review.
>>
>>
>> That's not the benefit, the benefit is that the code is clearer.
>
>
> I don't see that, and I don't like adding a check that I don't expect to be
> ever needed.

It's called self-documenting code; by adding a check for the NULL
pointer, we are stating that ce can be NULL, if we don't do that,
people reading that code would need to figure that out themselves.

> Or you could submit a patch on top that adds the check.

I already sent a patch that has that check.

http://article.gmane.org/gmane.comp.version-control.git/225972

-- 
Felipe Contreras

[PATCH v2 8/7] unpack-trees: document that pointer ce can be NULL

[René Scharfe]

From: Felipe Contreras <felipe.contreras@gmail.com>

If someone adds code that dereferences ce before it is freed without
checking for NULL it will crash sometimes.  Spare that person from
having to wonder about the reason.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
Signoff from http://article.gmane.org/gmane.comp.version-control.git/225972.
No signoff from me because I don't see the point of adding a check for
a developer that probably won't appear.

 unpack-trees.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 57b4074..f22bd89 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -605,7 +605,7 @@ static int unpack_nondirectories(int n, unsigned long mask,
 					o);
 		for (i = 0; i < n; i++) {
 			struct cache_entry *ce = src[i + o->merge];
-			if (ce != o->df_conflict_entry)
+			if (ce && ce != o->df_conflict_entry)
 				free(ce);
 		}
 		return rc;
-- 
1.8.3

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> I don't see that, and I don't like adding a check that I don't expect to be
>> ever needed.
>
> It's called self-documenting code; by adding a check for the NULL
> pointer, we are stating that ce can be NULL, if we don't do that,
> people reading that code would need to figure that out themselves.

People following the codepath to unpack_nondirectories() already
have seen enough to know what src[] means and very well know what
NULL in it means.  The only people possibly confused are those who
do not know free(NULL) is safe, isn't it?

Honestly speaking, I do not want such people to be touching this
part of the system.

Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges

[Felipe Contreras]

On Mon, Jun 3, 2013 at 12:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> I don't see that, and I don't like adding a check that I don't expect to be
>>> ever needed.
>>
>> It's called self-documenting code; by adding a check for the NULL
>> pointer, we are stating that ce can be NULL, if we don't do that,
>> people reading that code would need to figure that out themselves.
>
> People following the codepath to unpack_nondirectories() already
> have seen enough to know what src[] means and very well know what
> NULL in it means.  The only people possibly confused are those who
> do not know free(NULL) is safe, isn't it?

Wrong. I still do not know what src[] means, and I don't need to know,
I can see from the code that the cached entries there leak.

> Honestly speaking, I do not want such people to be touching this
> part of the system.

So we should make it more obfuscated?

-- 
Felipe Contreras