From: Junio C Hamano <gitster@pobox.com>
To: "Marc-André Lureau" <marcandre.lureau@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH/RFC 5/6] builtin-tag: add sort by date -D
Date: Sun, 22 Feb 2009 10:33:57 -0800 [thread overview]
Message-ID: <7vhc2mxqwa.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <e29894ca0902221006j3d602553x15807b41698f51a1@mail.gmail.com> (Marc-André Lureau's message of "Sun, 22 Feb 2009 20:06:45 +0200")
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 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.
> + 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.
next prev parent reply other threads:[~2009-02-22 18:35 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-02-22 18:06 [PATCH/RFC 5/6] builtin-tag: add sort by date -D Marc-André Lureau
2009-02-22 18:33 ` Junio C Hamano [this message]
2009-02-22 18:38 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vhc2mxqwa.fsf@gitster.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=marcandre.lureau@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).