From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sun, 05 Sep 2010 05:02:58 GMT
Subject: Re: [PATCH] Add test-string-list.c
Message-ID: <20100905050254.GA6134@burratino>
In-Reply-To: <4f798daaf4631e00927b455b77919d5bb8fade03.1283653854.git.tfransosi@gmail.com>

Hi Thiago,

Thiago Farina wrote:

> Add a simple test that demonstrates how to create and manipulate
> a list of strings using the string-list.h API.

Quick thoughts:

> --- /dev/null
> +++ b/test-string-list.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>

Git programs tend to start with 

 #include "cache.h"

or

 #include "git-compat-util.h"

to get all the portability niceties.

> +	print_string_list(&list, "");
> +
> +        int has_foo = string_list_has_string(&list, "foo");

Whitespace, declaration after statement... (see
Documentation/CodingGuidelines).

> +	if (has_foo != 1)
> +		error("List doesn't have foo.");

This does not exit with nonzero status when it fails.  You probably
wanted

	if (bad things)
		return error("problems!");

> +	string_list_clear(&list, 0);
> +	if (list.nr > 0)
> +		error("List is not clear.");

To make sure this example remains valid, wouldn't you want to include
a caller in the t/ directory so it can be automatically run?  (See
t/README.)

Thoughts separate from the code:

 * it is probably worth mentioning Documentation/technical/api-string-list.txt
   for people who do not know about it.

 * for this to be useful as a test I think one has to sort of believe
   that it can break.  That is, a test of something this basic (which
   is already demonstrated and exercised by code throughout git, after
   all) would tend to be especially devious.

 * api-string-list.txt does not mention the STRING_LIST_INIT macros
   you introduced.  Maybe that would be worth improving.

Regards,
Jonathan