Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Jun 13, 2014, 11:45 UTC
Message-ID: <539AE476.4030205@drmicha.warpmail.net>
In-Reply-To: <20140613111945.GC14066@sigill.intra.peff.net>

Jeff King venit, vidit, dixit 13.06.2014 13:19:

Show 11 quoted lines

> On Fri, Jun 13, 2014 at 12:42:45PM +0200, Michael J Gruber wrote:
> 
>> +
>> +	free(signature_check.gpg_output);
>> +	free(signature_check.gpg_status);
>> +	free(signature_check.signer);
>> +	free(signature_check.key);
>> +	return signature_check.result != 'G';
>> +}
> 
> How about .payload here?

I sneekily fix this in 6/6... I thought 3/6 is on next already, too late for a real v2. Otherwise I would put 6/6 before everything else.

Show 26 quoted lines

>> +	type = sha1_object_info(sha1, NULL);
>> +	if (type != OBJ_COMMIT)
>> +		return error("%s: cannot verify a non-commit object of type %s.",
>> +				name, typename(type));
>> +
>> +	buf = read_sha1_file(sha1, &type, &size);
>> +	if (!buf)
>> +		return error("%s: unable to read file.", name);
> 
> I think you can drop the sha1_object_info call and just check "type"
> from read_sha1_file (they _should_ agree, but if they do not, I'd rather
> pay attention to the one that came along with the buffer). And this is
> the uncommon error path, so expanding the object into memory before we
> die is not a big deal.
> 
> Should this peel to a commit if given a tag? I'd say probably. I know
> you raised the issue elsewhere of keeping things simple, but I think if
> you are calling verify-commit, you know you want a commit, and we should
> treat the argument as a commit-ish. Anyway, if you go that route, then
> lookup_commit_or_die is probably what you want.
> 
> Also, minor nit, but we typically do not end the error messages with a
> full stop (we've been rather inconsistent in the past, but these days
> seem to mostly settle on no punctuation).
> 
> -Peff

Both of these issues actually come for following verify-tag as closely as possible. If 3 is not applied already, I should do away with sha1_object_info.

About the peeling I'm not so sure, since there's a difference between a signed tag pointing to a commit and a signed commit. Since verify-{tag,commit} are bare metal plumbing, I would expect callers to use <rev>^{commit} explicitly if they don't care how <rev> peels to a commit.

Michael