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

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.