Re: [PATCH v3 1/4] replace: add --graft option

Christian Couder [off-list ref] writes:

+static int create_graft(int argc, const char **argv, int force)
+{
+	unsigned char old[20], new[20];
+	const char *old_ref = argv[0];
+	struct commit *commit;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf new_parents = STRBUF_INIT;
+	const char *parent_start, *parent_end;
+	int i;
+
+	if (get_sha1(old_ref, old) < 0)
+		die(_("Not a valid object name: '%s'"), old_ref);
+	commit = lookup_commit_or_die(old, old_ref);

Not a problem with this patch, but the above sequence somehow makes
me wonder if lookup-commit-or-die is a good API for this sort of
thing.  Wouldn't it be more natural if it took not "unsigned char
old[20]" but anything that would be understood by get_sha1()?

It could be that this particular caller is peculiar and all the
existing callers are happy, though.  I didn't "git grep" to spot
patterns in existing callers.

+	if (read_sha1_commit(old, &buf))
+		die(_("Invalid commit: '%s'"), old_ref);
+	/* make sure the commit is not corrupt */
+	if (parse_commit_buffer(commit, buf.buf, buf.len))
+		die(_("Could not parse commit: '%s'"), old_ref);

It is unclear to me what you are trying to achieve with these two.
If the earlier lookup-commit has returned a commit object that has
already been parsed, parse_commit_buffer() would not check anything,
would it?

A typical sequence would look more like this:

    commit = lookup_commit(...);
    if (parse_commit(commit))
	oops there is an error;
    /* otherwise */
    use(commit->buffer);

without reading a commit object using low-level read-sha1-file
interface yourself, no?

On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

+static int create_graft(int argc, const char **argv, int force)
+{
+     unsigned char old[20], new[20];
+     const char *old_ref = argv[0];
+     struct commit *commit;
+     struct strbuf buf = STRBUF_INIT;
+     struct strbuf new_parents = STRBUF_INIT;
+     const char *parent_start, *parent_end;
+     int i;
+
+     if (get_sha1(old_ref, old) < 0)
+             die(_("Not a valid object name: '%s'"), old_ref);
+     commit = lookup_commit_or_die(old, old_ref);

Not a problem with this patch, but the above sequence somehow makes
me wonder if lookup-commit-or-die is a good API for this sort of
thing.  Wouldn't it be more natural if it took not "unsigned char
old[20]" but anything that would be understood by get_sha1()?

It could be that this particular caller is peculiar and all the
existing callers are happy, though.  I didn't "git grep" to spot
patterns in existing callers.

lookup_commit_or_die() looked like a good API to me because I saw that
it checked a lot of things and die in case of problems, which could
make the patch shorter.

quoted

+     if (read_sha1_commit(old, &buf))
+             die(_("Invalid commit: '%s'"), old_ref);
+     /* make sure the commit is not corrupt */
+     if (parse_commit_buffer(commit, buf.buf, buf.len))
+             die(_("Could not parse commit: '%s'"), old_ref);

It is unclear to me what you are trying to achieve with these two.
If the earlier lookup-commit has returned a commit object that has
already been parsed, parse_commit_buffer() would not check anything,
would it?

Yeah, you are right. I missed the fact that lookup_commit_or_die()
calls parse_object() which itself calls read_sha1_file() and then
parse_object_buffer() which calls parse_commit_buffer().

Here is a backtrace that shows this:

#0  parse_commit_buffer (item=0x8597b0, buffer=0x851730, size=228) at
commit.c:251
#1  0x00000000004fa215 in parse_object_buffer (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234",
type=OBJ_COMMIT, size=228,
    buffer=0x851730, eaten_p=0x7fffffffdacc) at object.c:198
#2  0x00000000004fa50a in parse_object (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234") at
object.c:264
#3  0x00000000004a89ef in lookup_commit_reference_gently
(sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234", quiet=0) at
commit.c:38
#4  0x00000000004a8a48 in lookup_commit_reference (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234") at
commit.c:47
#5  0x00000000004a8a67 in lookup_commit_or_die (sha1=0x7fffffffdbf0
"\t>A\247\235J\213\376<u\212\226\311^[\371\343^\330\234",
    ref_name=0x7fffffffe465
"093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c") at commit.c:52
#6  0x000000000047f89a in create_graft (argc=1, argv=0x7fffffffe130,
refdir=0x0, force=0) at builtin/replace.c:353
#7  0x000000000047ff71 in cmd_replace (argc=1, argv=0x7fffffffe130,
prefix=0x0) at builtin/replace.c:461
#8  0x0000000000405441 in run_builtin (p=0x7eee90, argc=3,
argv=0x7fffffffe130) at git.c:314
#9  0x000000000040563a in handle_builtin (argc=3, argv=0x7fffffffe130)
at git.c:487
#10 0x0000000000405754 in run_argv (argcp=0x7fffffffe01c,
argv=0x7fffffffe020) at git.c:533
#11 0x00000000004058f9 in main (argc=3, av=0x7fffffffe128) at git.c:616

A typical sequence would look more like this:

    commit = lookup_commit(...);
    if (parse_commit(commit))
        oops there is an error;
    /* otherwise */
    use(commit->buffer);

without reading a commit object using low-level read-sha1-file
interface yourself, no?

Yeah, or I could just rely on the fact that lookup_commit_or_die()
already parses the commit, with something like this:

      if (get_sha1(old_ref, old) < 0)
              die(_("Not a valid object name: '%s'"), old_ref);

      /* parse the commit buffer to make sure the commit is not corrupt */
      commit = lookup_commit_or_die(old, old_ref);

      /* find existing parents */
      parent_start = buf.buf;
      parent_start += 46; /* "tree " + "hex sha1" + "\n" */
      parent_end = parent_start;
...

Thanks,
Christian.

On Fri, Jun 6, 2014 at 5:29 PM, Christian Couder
[off-list ref] wrote:

Yeah, or I could just rely on the fact that lookup_commit_or_die()
already parses the commit, with something like this:

      if (get_sha1(old_ref, old) < 0)
              die(_("Not a valid object name: '%s'"), old_ref);

      /* parse the commit buffer to make sure the commit is not corrupt */
      commit = lookup_commit_or_die(old, old_ref);

      /* find existing parents */
      parent_start = buf.buf;
      parent_start += 46; /* "tree " + "hex sha1" + "\n" */
      parent_end = parent_start;

This last part should be:

        /* find existing parents */
        strbuf_addstr(&buf, commit->buffer);
        parent_start = buf.buf;
        parent_start += 46; /* "tree " + "hex sha1" + "\n" */
        parent_end = parent_start;
...

I will send an updated patch series soon.

On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder
[off-list ref] wrote:

        /* find existing parents */
        strbuf_addstr(&buf, commit->buffer);

Unfortunately, it looks like the above will not work if the commit->buffer
contains an embedded NUL. I wonder if it is a real problem or not.

On Sun, Jun 08, 2014 at 08:49:45AM +0200, Christian Couder wrote:

On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder
[off-list ref] wrote:

quoted

        /* find existing parents */
        strbuf_addstr(&buf, commit->buffer);

Unfortunately, it looks like the above will not work if the commit->buffer
contains an embedded NUL. I wonder if it is a real problem or not.

I ran into a similar problem recently[1] and have been pondering
solutions to know the size of commit->buffer. What I've been come up
with is:

  1. Look up the object size via sha1_object_info. Besides being
     inefficient (which probably does not matter for you here, but might
     for using commit->buffer in a traversal), it strikes me as
     inelegant; is it possible for commit->buffer to ever disagree in
     size with the results of sha1_object_info, and if so, what happens?

  2. Add an extra member "len" to "struct commit". This is simple, but
     bloats "struct commit", which may have a performance impact for
     things like rev-list, where the field will be unused.

  3. Store the length of objects as a size_t, exactly sizeof(size_t)
     bytes before the object buffer. Provide a macro:

       #define OBJECT_SIZE(buf) (((size_t *)(buf))[-1])

     to access it. Most callers can just use the buffer as-is, but
     anybody who calls free() would need to be adjusted to use a special
     "object_free".

  4. Keep a static commit_slab that points to the length for each parsed
     commit. We pay the same memory cost as (2), but as it's not part of
     the struct, the cache effects are minimized.

-Peff

[1] http://article.gmane.org/gmane.comp.version-control.git/250480

On Sun, Jun 08, 2014 at 07:23:33AM -0400, Jeff King wrote:

  4. Keep a static commit_slab that points to the length for each parsed
     commit. We pay the same memory cost as (2), but as it's not part of
     the struct, the cache effects are minimized.

I think I favor this solution, which would look something like this:

-- >8 --
Subject: [PATCH] commit: add slab for commit buffer size

We store the commit object buffer for later reuse as
commit->buffer. However, since we store only a pointer, we
must treat the result as a NUL-terminated string. This is
generally OK for pretty-printing, but could be a problem for
other uses.

Adding a "len" member to "struct commit" would solve this,
but at the cost of bloating the struct even for callers who
do not care about the size or buffer (e.g., traversals like
rev-list or merge-base). Instead, let's use a commit_slab so
that the memory is used only when save_commit_buffer is in
effect (and even then, it should have less cache impact on
most uses of "struct commit").

Signed-off-by: Jeff King <redacted>
---
I think it would make sense to actually take this one step further,
though, and move commit->buffer into the slab, as well. That has two
advantages:

  1. It further decreases the size of "struct commit" for callers who do
     not use save_commit_buffer.

  2. It ensures that no new callers crop up who set "commit->buffer" but
     to not save the size in the slab (you can see in the patch below
     that I had to modify builtin/blame.c, which (ab)uses
     commit->buffer).

It would be more disruptive to existing callers, but I think the end
result would be pretty clean. The API would be something like:

  /* attach buffer to commit */
  set_commit_buffer(struct commit *, void *buf, unsigned long size);

  /* get buffer, either from slab cache or by calling read_sha1_file */
  void *get_commit_buffer(struct commit *, unsigned long *size);

  /* free() an allocated buffer from above, noop for cached buffer */
  void unused_commit_buffer(struct commit *, void *buf);

  /* drop saved commit buffer to free memory */
  void free_commit_buffer(struct commit *);

The "get" function would serve the existing callers in pretty.c, as well
as the one I'm adding elsewhere in show_signature. And it should work as
a drop-in read_sha1_file/free replacement for you here.

 builtin/blame.c |  2 +-
 commit.c        | 13 ++++++++++++-
 commit.h        |  1 +
 object.c        |  2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..1945ea4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 		    ident, ident, path,
 		    (!contents_from ? path :
 		     (!strcmp(contents_from, "-") ? "standard input" : contents_from)));
-	commit->buffer = strbuf_detach(&msg, NULL);
+	set_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);
 
 	if (!contents_from || strcmp("-", contents_from)) {
 		struct stat st;

diff --git a/commit.c b/commit.c
index f479331..71143ed 100644
--- a/commit.c
+++ b/commit.c

@@ -302,6 +302,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
 	return 0;
 }
 
+define_commit_slab(commit_size_slab, unsigned long);
+static struct commit_size_slab commit_size;
+
+void set_commit_buffer(struct commit *item, void *buffer, unsigned long size)
+{
+	if (!commit_size.stride)
+		init_commit_size_slab(&commit_size);
+	*commit_size_slab_at(&commit_size, item) = size;
+	item->buffer = buffer;
+}
+
 int parse_commit(struct commit *item)
 {
 	enum object_type type;

@@ -324,7 +335,7 @@ int parse_commit(struct commit *item)
 	}
 	ret = parse_commit_buffer(item, buffer, size);
 	if (save_commit_buffer && !ret) {
-		item->buffer = buffer;
+		set_commit_buffer(item, buffer, size);
 		return 0;
 	}
 	free(buffer);

diff --git a/commit.h b/commit.h
index a9f177b..7704ab2 100644
--- a/commit.h
+++ b/commit.h

@@ -48,6 +48,7 @@ struct commit *lookup_commit_reference_by_name(const char *name);
 struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name);
 
 int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);
+void set_commit_buffer(struct commit *item, void *buffer, unsigned long size);
 int parse_commit(struct commit *item);
 void parse_commit_or_die(struct commit *item);
 

diff --git a/object.c b/object.c
index 57a0890..c1c6a24 100644
--- a/object.c
+++ b/object.c

@@ -198,7 +198,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 			if (parse_commit_buffer(commit, buffer, size))
 				return NULL;
 			if (!commit->buffer) {
-				commit->buffer = buffer;
+				set_commit_buffer(commit, buffer, size);
 				*eaten_p = 1;
 			}
 			obj = &commit->object;

-- 
2.0.0.729.g520999f

On Sun, Jun 08, 2014 at 08:04:39AM -0400, Jeff King wrote:

Side note: this is wrong, as the fake commit created by blame here does
not have its "index" field set. This is a bug waiting to happen the
first time somebody uses a slab in builtin/blame.c. It looks like
merge-recursive does the same thing in make_virtual_commit.

We probably want to provide a function to allocate a commit, including
the index, and use it consistently.

I'll try to work up a series doing that, and the fuller slab API I
mentioned in the previous message, but probably not until tomorrow.

-Peff