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

On Tue, Feb 07, 2012 at 02:12:02PM -0500, Jeff King wrote:

On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:

quoted

quoted

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.

I'm not sure I understand. Wouldn't you match lightweight tags by the
sha1 they point at? Something like:

I think the following would show the pointed at tag too.
  $ git tag my-v1.7.9 v1.7.9
  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9
  v1.7.9

vs.

  $ ./git-tag -l --points-at v1.7.9
  my-v1.7.9

I found that I had to filter matching refnames.

 static int tag_points_at(struct sha1_array *sa,
                          const unsigned char *sha1)
 {
         struct object *obj;

         /* Lightweight tag of an interesting sha1? */
         if (sha1_array_lookup(sa, sha1) >= 0)
                 return 1;

         /* Otherwise, maybe a tag object pointing to an interesting sha1 */
         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;
}

quoted

quoted

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;
	}

Yes, exactly.

quoted

quoted

quoted

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

Right. Though without flags, you can probably just use the OPT_CALLBACK
wrapper, like:

 OPT_CALLBACK(0, "points-at", &points_at, "object",
              "print only annotated|signed tags of the object",
              parse_opt_points_at)

Note that if you are going to handle lightweight tags, that description
should probably be updated.

-Peff

-- 
TomG