Re: [PATCH] http: Add Accept-Language header if possible

I agree that a list of char* is enough for language_tags.

Thanks for your review and patch. I'll apply your patch and send v9.

On Wed, Jan 28, 2015 at 3:15 PM, Junio C Hamano [off-list ref] wrote:

On Tue, Jan 27, 2015 at 3:34 PM, Junio C Hamano [off-list ref] wrote:

quoted

Yi EungJun [off-list ref] writes:

quoted

+
+             sprintf(q_format, ";q=0.%%0%dd", decimal_places);
+
+             strbuf_addstr(buf, "Accept-Language: ");
+
+             for(i = 0; i < num_langs; i++) {
+                     if (i > 0)
+                             strbuf_addstr(buf, ", ");
+
+                     strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));

This is not wrong per-se, but it looks somewhat convoluted to me.
...

Actually, this is wrong, isn't it?

strbuf_detach() removes the language_tags[i].buf from the strbuf,
and the caller now owns that piece of memory. Then strbuf_addstr()
appends a copy of that string to buf, and the piece of memory
that was originally held by language_tags[i].buf is now lost forever.

This is leaking.

quoted

quoted

+     /* free language tags */
+     for(i = 0; i < num_langs; i++) {
+             strbuf_release(&language_tags[i]);
+     }

... because this loop does not free memory for earlier parts of language_tags[].

quoted

I am wondering if using strbuf for each of the language_tags[] is
even necessary.  How about doing it this way instead?

And I think my counter-proposal does not leak (as it does not us strbuf for
language_tags[] anymore).

quoted

 http.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/http.c b/http.c
index 6111c6a..db591b3 100644
--- a/http.c
+++ b/http.c

@@ -1027,7 +1027,7 @@ static void write_accept_language(struct strbuf *buf)
        const int MAX_DECIMAL_PLACES = 3;
        const int MAX_LANGUAGE_TAGS = 1000;
        const int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;
-       struct strbuf *language_tags = NULL;
+       char **language_tags = NULL;
        int num_langs = 0;
        const char *s = get_preferred_languages();
        int i;

@@ -1053,9 +1053,7 @@ static void write_accept_language(struct strbuf *buf)
                if (tag.len) {
                        num_langs++;
                        REALLOC_ARRAY(language_tags, num_langs);
-                       strbuf_init(&language_tags[num_langs - 1], 0);
-                       strbuf_swap(&tag, &language_tags[num_langs - 1]);
-
+                       language_tags[num_langs - 1] = strbuf_detach(&tag, NULL);
                        if (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */
                                break;
                }

@@ -1070,13 +1068,12 @@ static void write_accept_language(struct strbuf *buf)

                /* add '*' */
                REALLOC_ARRAY(language_tags, num_langs + 1);
-               strbuf_init(&language_tags[num_langs], 0);
-               strbuf_addstr(&language_tags[num_langs++], "*");
+               language_tags[num_langs++] = "*"; /* it's OK; this won't be freed */

                /* compute decimal_places */
                for (max_q = 1, decimal_places = 0;
-                               max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;
-                               decimal_places++, max_q *= 10)
+                    max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;
+                    decimal_places++, max_q *= 10)
                        ;

                sprintf(q_format, ";q=0.%%0%dd", decimal_places);

@@ -1087,7 +1084,7 @@ static void write_accept_language(struct strbuf *buf)
                        if (i > 0)
                                strbuf_addstr(buf, ", ");

-                       strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));
+                       strbuf_addstr(buf, language_tags[i]);

                        if (i > 0)
                                strbuf_addf(buf, q_format, max_q - i);

@@ -1101,10 +1098,9 @@ static void write_accept_language(struct strbuf *buf)
                }
        }

-       /* free language tags */
-       for(i = 0; i < num_langs; i++) {
-               strbuf_release(&language_tags[i]);
-       }
+       /* free language tags -- last one is a static '*' */
+       for(i = 0; i < num_langs - 1; i++)
+               free(language_tags[i]);
        free(language_tags);
 }