Christian Couder <chriscool@xxxxxxxxxxxxx> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html