Re: [PATCH 16/21] Introduce optional "keywords" on tag objects

From: Johan Herland <johan@herland.net>
Date: Jun 9, 2007, 22:00 UTC
Message-ID: <200706100000.45476.johan@herland.net>
In-Reply-To: <81b0412b0706091452q2957540dy95fbf13ebd89ca1f@mail.gmail.com>

On Saturday 09 June 2007, Alex Riesen wrote:

Show 6 quoted lines

> On 6/9/07, Johan Herland <johan@herland.net> wrote:
> > This patch introduces a new optional header line to the tag object, called
> > "keywords". The "keywords" line may contain a comma-separated list of
> > custom keywords associated with the tag object.
> 
> What is the character set for the keywords?

Hmm. Same as for the "tag" line, and the rest of the tag object, I guess. I agree this should probably be specified. What are the rules for other git objects?

Show 11 quoted lines

> > +                       for (i = 0; i < keywords_len; ++i) {
> > +                               unsigned char c = keywords_line[i];
> > +                               if (c == ',' && keywords_line[i + 1] == ',')
> > +                                       /* consecutive commas */
> > +                                       return error("Tag object (@ char "
> > +                                               PD_FMT "): Found empty keyword",
> > +                                               keywords_line + i - data);
> > +                               if (c > ' ' && c != 0x7f)
> > +                                       continue;
> 
> And what is so special about 0x7f?

Isn't DEL a control char?

> > +               if (keywords_len) { /* optional keywords string was given */
> > +                       item->keywords = xmalloc(keywords_len + 1);
> 
> Who frees the keywords and what's wrong with strndup?

The code that should free the tag name should also free the keywords. No code appear to do this at the moment (regardless of whether you use my patch series or not), which is a shame. We should make sure all objects are properly deallocated. This is currently a general problem in git, isn't it?

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net