Christian Couder <> 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 */

without reading a commit object using low-level read-sha1-file
interface yourself, no?
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to