| Message ID | 20190509213229.GN15837@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/14] cmd_{read,write}_tree: rename "unused" variable that is used | expand |
On 5/9/2019 5:32 PM, Jeff King wrote: > Instead, let's do our type check by loading the object via > parse_object(). That will attach the buffer to the struct so it can be > used later by check_commit_signature(). And it ensures that > lookup_commit() will return something sane. This is a good idea. > -static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned long size, unsigned flags) > +static int run_gpg_verify(struct commit *commit, unsigned flags) > { > struct signature_check signature_check; > int ret; > > memset(&signature_check, 0, sizeof(signature_check)); > > - ret = check_commit_signature(lookup_commit(the_repository, oid), > - &signature_check); > + ret = check_commit_signature(commit, &signature_check); > print_signature_buffer(&signature_check, flags); Bonus drop of the_repository. > > signature_check_clear(&signature_check); > @@ -38,26 +37,20 @@ static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned > > static int verify_commit(const char *name, unsigned flags) > { > - enum object_type type; > struct object_id oid; > - char *buf; > - unsigned long size; > - int ret; > + struct object *obj; > > if (get_oid(name, &oid)) > return error("commit '%s' not found.", name); > > - buf = read_object_file(&oid, &type, &size); > - if (!buf) > + obj = parse_object(the_repository, &oid); ...and it is back, but "higher" up. That's fine. > + if (!obj) > return error("%s: unable to read file.", name); > - if (type != OBJ_COMMIT) > + if (obj->type != OBJ_COMMIT) > return error("%s: cannot verify a non-commit object of type %s.", > - name, type_name(type)); > - > - ret = run_gpg_verify(&oid, buf, size, flags); > + name, type_name(obj->type)); > > - free(buf); > - return ret; > + return run_gpg_verify((struct commit *)obj, flags); > } You pointed out that you thought this patch was subtle. I agree that there is a slightly changed functionality, but it seems to be improving a possibly wrong behavior. Thanks, -Stolee
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c index 7772c07ed7..4b9e823f8f 100644 --- a/builtin/verify-commit.c +++ b/builtin/verify-commit.c @@ -21,15 +21,14 @@ static const char * const verify_commit_usage[] = { NULL }; -static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned long size, unsigned flags) +static int run_gpg_verify(struct commit *commit, unsigned flags) { struct signature_check signature_check; int ret; memset(&signature_check, 0, sizeof(signature_check)); - ret = check_commit_signature(lookup_commit(the_repository, oid), - &signature_check); + ret = check_commit_signature(commit, &signature_check); print_signature_buffer(&signature_check, flags); signature_check_clear(&signature_check); @@ -38,26 +37,20 @@ static int run_gpg_verify(const struct object_id *oid, const char *buf, unsigned static int verify_commit(const char *name, unsigned flags) { - enum object_type type; struct object_id oid; - char *buf; - unsigned long size; - int ret; + struct object *obj; if (get_oid(name, &oid)) return error("commit '%s' not found.", name); - buf = read_object_file(&oid, &type, &size); - if (!buf) + obj = parse_object(the_repository, &oid); + if (!obj) return error("%s: unable to read file.", name); - if (type != OBJ_COMMIT) + if (obj->type != OBJ_COMMIT) return error("%s: cannot verify a non-commit object of type %s.", - name, type_name(type)); - - ret = run_gpg_verify(&oid, buf, size, flags); + name, type_name(obj->type)); - free(buf); - return ret; + return run_gpg_verify((struct commit *)obj, flags); } static int git_verify_commit_config(const char *var, const char *value, void *cb)
The buf/len parameters of run_gpg_verify() have never been used since the function was added in d07b00b7f3 (verify-commit: scriptable commit signature verification, 2014-06-23). Instead, check_commit_signature() accesses the commit struct directly. Worse, we read the whole object just to check its type and do not attach it to the "struct commit". Meaning we end up loading the object from disk twice for no good reason. And to further confuse matters, our type check is comes from what we read from disk, but we later assume that lookup_commit() will return non-NULL. This might not be true if some other object previously referenced the same oid as a non-commit (though this may be impossible to trigger in practice since we don't generally parse any other objects in this command). Instead, let's do our type check by loading the object via parse_object(). That will attach the buffer to the struct so it can be used later by check_commit_signature(). And it ensures that lookup_commit() will return something sane. And then we can just drop the unused "buf" and "len" parameters entirely. Signed-off-by: Jeff King <peff@peff.net> --- builtin/verify-commit.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)