Re: [PATCHv2] tag: add --points-at list option

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

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

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

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