On Tue, Feb 07, 2012 at 11:05:27AM -0500, Jeff King wrote:
>On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:
>
>> +struct points_at {
>> + struct points_at *next;
>> + unsigned char *sha1;
>> +};
>
>Would using sha1_array save us from having to create our own data
>structure? As a bonus, it can do O(lg n) lookups, though I seriously
>doubt anyone will provide a large number of "--points-at".
Thanks, but I now realize that I also need to save the pointed at
refname to detect lightweight tags that have matching sha's but
different names.
>> +static void free_points_at (struct points_at *points_at)
>> +{
>> + while (points_at) {
>> + struct points_at *next = points_at->next;
>> + free(points_at->sha1);
>> + free(points_at);
>> + points_at = next;
>> + }
>> +}
>
>Then this could go away in favor of sha1_array_clear.
>
>> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)
>> +{
>> + struct points_at *new, **opt_value = (struct points_at **)opt->value;
>> + unsigned char *sha1;
>> +
>> + if (!arg)
>> + return error(_("missing <object>"));
>> + new = xmalloc(sizeof(struct points_at));
>> + sha1 = xmalloc(20);
>> + if (get_sha1(arg, sha1)) {
>> + free(new);
>> + free(sha1);
>> + return error(_("malformed object name '%s'"), arg);
>> + }
>> + new->sha1 = sha1;
>> + new->next = *opt_value;
>> + *opt_value = new;
>> + return 0;
>> +}
>
>And this can drop all of the memory management bits, like:
>
> unsigned char sha1[20];
>
> if (!arg)
> return error(_("missing <object>"));
> if (get_sha1(arg, sha1))
> return error(_("malformed object name '%s'"), arg);
> sha1_array_append(opt->value, sha1);
> return 0;
>
>Also, should you check "unset"? When we have options that build a list,
>usually doing "--no-foo" will clear the list. E.g., this:
>
> git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz
>
>should look only for "baz".
Ahh, so I just need to:
if (unset) {
if (*opt_value)
free_points_at(*opt_value);
*opt_value = NULL;
return 0;
}
>> +static struct points_at *match_points_at(struct points_at *points_at,
>> + const unsigned char *sha1)
>> +{
>> + char *buf;
>> + struct tag *tag;
>> + unsigned long size;
>> + enum object_type type;
>> +
>> + buf = read_sha1_file(sha1, &type, &size);
>> + if (!buf)
>> + return NULL;
>> + if (type != OBJ_TAG
>> + || (tag = lookup_tag(sha1), !tag)
>> + || parse_tag_buffer(tag, buf, size) < 0) {
>> + free(buf);
>> + return NULL;
>> + }
>> + while (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))
>> + points_at = points_at->next;
>> + free(buf);
>> + return points_at;
>> +}
>
>Sorry, I threw a lot of object lookup code at you last time, so I think
>my point may have been lost in the noise. But I think this is slightly
>nicer as:
>
> static int tag_points_at(struct sha1_array *sa,
> const unsigned char *sha1)
> {
> struct object *obj = parse_object(sha1);
> if (!obj)
> return 0; /* or probably we should even just die() */
> if (obj->type != OBJ_TAG)
> return 0;
> if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)
> return 0;
> return 1;
> }
>
>I.e., using parse_object lets you avoid dealing with memory management
>yourself. And as a bonus, it will reuse the cached information if you
>happen to have already parsed that object (not likely in typical
>repositories, but a huge win in certain pathological cases, like repos
>storing shared objects and refs for a large number of forks).
Aye
>> + {
>> + OPTION_CALLBACK, 0, "points-at", &points_at, "object",
>> + "print only annotated|signed tags of the object",
>> + PARSE_OPT_LASTARG_DEFAULT,
>> + parse_opt_points_at, (intptr_t)NULL,
>> + },
>
>I think you can drop the LASTARG_DEFAULT here, as it is no longer
>optional, no?
You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?
Thanks,
TomG
--
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