Re: [PATCH 03/15] do not create struct commit with xcalloc

2014-06-10 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 In both blame and merge-recursive, we sometimes create a
 fake commit struct for convenience (e.g., to represent the
 HEAD state as if we would commit it). By allocating
 ourselves rather than using alloc_commit_node, we do not
 properly set the index field of the commit. This can
 produce subtle bugs if we then use commit-slab on the
 resulting commit, as we will share the 0 index with
 another commit.

The change I see here is all good, but I wonder if it is too late to
start the real index from 1 and catch anything that has 0 index with
a BUG(do not hand-allocate a commit).

 We can fix this by using alloc_commit_node() to allocate.
 Note that we cannot free the result, as it is part of our
 commit allocator. However, both cases were already leaking
 the allocated commit anyway, so there's nothing to fix up.

 Signed-off-by: Jeff King p...@peff.net
 ---
  builtin/blame.c   | 2 +-
  merge-recursive.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

 diff --git a/builtin/blame.c b/builtin/blame.c
 index a52a279..d6056a5 100644
 --- a/builtin/blame.c
 +++ b/builtin/blame.c
 @@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct 
 diff_options *opt,
   struct strbuf msg = STRBUF_INIT;
  
   time(now);
 - commit = xcalloc(1, sizeof(*commit));
 + commit = alloc_commit_node();
   commit-object.parsed = 1;
   commit-date = now;
   commit-object.type = OBJ_COMMIT;
 diff --git a/merge-recursive.c b/merge-recursive.c
 index cab16fa..2b37d42 100644
 --- a/merge-recursive.c
 +++ b/merge-recursive.c
 @@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, 
 struct tree *two,
  
  static struct commit *make_virtual_commit(struct tree *tree, const char 
 *comment)
  {
 - struct commit *commit = xcalloc(1, sizeof(struct commit));
 + struct commit *commit = alloc_commit_node();
   struct merge_remote_desc *desc = xmalloc(sizeof(*desc));
  
   desc-name = comment;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/15] do not create struct commit with xcalloc

2014-06-10 Thread Jeff King 
On Tue, Jun 10, 2014 at 02:35:48PM -0700, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  In both blame and merge-recursive, we sometimes create a
  fake commit struct for convenience (e.g., to represent the
  HEAD state as if we would commit it). By allocating
  ourselves rather than using alloc_commit_node, we do not
  properly set the index field of the commit. This can
  produce subtle bugs if we then use commit-slab on the
  resulting commit, as we will share the 0 index with
  another commit.
 
 The change I see here is all good, but I wonder if it is too late to
 start the real index from 1 and catch anything that has 0 index with
 a BUG(do not hand-allocate a commit).

Yeah, I had a similar thought while writing the series. I do not think
it is too late at all. It would just waste one commit's worth of memory
in the slab, but that is not a big deal. Other than slab_at, no callers
should care.

It would cost an extra conditional in each call to slab_at. That
probably doesn't matter, though the original goal was to make slab_at as
fast as an indirect pointer dereference (it's not quite these days,
though, as we have to do a little division to find the right section of
the slab).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/15] do not create struct commit with xcalloc

2014-06-09 Thread Jeff King 
In both blame and merge-recursive, we sometimes create a
fake commit struct for convenience (e.g., to represent the
HEAD state as if we would commit it). By allocating
ourselves rather than using alloc_commit_node, we do not
properly set the index field of the commit. This can
produce subtle bugs if we then use commit-slab on the
resulting commit, as we will share the 0 index with
another commit.

We can fix this by using alloc_commit_node() to allocate.
Note that we cannot free the result, as it is part of our
commit allocator. However, both cases were already leaking
the allocated commit anyway, so there's nothing to fix up.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/blame.c   | 2 +-
 merge-recursive.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..d6056a5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
struct strbuf msg = STRBUF_INIT;
 
time(now);
-   commit = xcalloc(1, sizeof(*commit));
+   commit = alloc_commit_node();
commit-object.parsed = 1;
commit-date = now;
commit-object.type = OBJ_COMMIT;
diff --git a/merge-recursive.c b/merge-recursive.c
index cab16fa..2b37d42 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, 
struct tree *two,
 
 static struct commit *make_virtual_commit(struct tree *tree, const char 
*comment)
 {
-   struct commit *commit = xcalloc(1, sizeof(struct commit));
+   struct commit *commit = alloc_commit_node();
struct merge_remote_desc *desc = xmalloc(sizeof(*desc));
 
desc-name = comment;
-- 
2.0.0.729.g520999f

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html