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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2014, 21:49 UTC
Message-ID: <xmqqfvjjrpq9.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140604194353.23903.89933.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 13 quoted lines

> +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.

Show 5 quoted lines

> +	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?