On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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.
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.
>> + 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.
--
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