Re: [PATCH/RFC 5/6] builtin-tag: add sort by date -D

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2009, 18:33 UTC
Message-ID: <7vhc2mxqwa.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <e29894ca0902221006j3d602553x15807b41698f51a1@mail.gmail.com>

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

Show 15 quoted lines

> Signed-off-by: Marc-Andre Lureau <marcandre.lureau@gmail.com>
> ---
>  builtin-tag.c |  162 +++++++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 129 insertions(+), 33 deletions(-)
>
> diff --git a/builtin-tag.c b/builtin-tag.c
> index 01e7374..8ff9d03 100644
> --- a/builtin-tag.c
> +++ b/builtin-tag.c
> @@ -16,7 +16,7 @@
>  static const char * const git_tag_usage[] = {
>  	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
>  	"git tag -d <tagname>...",
> -	"git tag -l [-n[<num>]] [<pattern>]",
> +	"git tag -l [-n[<num>] -D] [<pattern>]",

Please don't use a short-and-sweet "-D" for something whose usefulness is not proven yet. Especially this risks grief from typo-confusion with the existing "-d" option that is destructive.

Show 13 quoted lines

> +		if (object->type != OBJ_TAG) {
> +			struct light_tag *light_tag;
> +			struct object *o;
> +
> +			o = xmalloc(sizeof(struct light_tag));
> +			light_tag = (struct light_tag*)o;
> +			o->parsed = 1;
> +			o->used = 0;
> +			o->type = OBJ_LIGHT_TAG;
> +			o->flags = 0;
> +			light_tag->tagged = object;
> +			light_tag->refname = xstrdup(refname);
> +			object = o;

I really do not like this. The only place you need a stand-in tag object is inside this "sort tag objects and commits together", and you cannot even handle lightweight tags that point at blobs or trees sanely with this code anyway. It is not a good excuse to contaminate the object layer.

I think it might be a lot more sensible to introduce a structure like this:

	struct tag_entry {
        	struct object *object;
                unsigned long date_for_sorting;
	};

and allocate and queue this structure in your for_each_ref callback function, instead of the low-level objects. If object *is* not a tag, you can at that point find a suitable timestamp to stuff in date_for_sorting (and if it is a tag, you can find a tagger field and parse the date into date_for_sorting, which implies you do not necessarily need your patch 2/6 either and we can keep sizeof(struct tag) to the minimum as before).

Then your sort and output functions can sort and iterate over a list of this structure.

You still need to think about what to do with lightweight tag that points at a blob or a tree, though.