Re: [PATCH] Add test-string-list.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jonathan Nieder <jrnieder@xxxxxxxxx> writes:

> Git programs tend to start with 
>
>  #include "cache.h"
>
> or
>
>  #include "git-compat-util.h"
>
> to get all the portability niceties.

Quote Documentation/CodingGuidelines here, too.  And use of these headers
is not merely "niceties"--some exotic platforms tend to want standard
system headers in particular inclusion order and what git-compat-util.h
does is the result of us painfully finding these rules out over years.

> 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.

 * this may be built but nothing exercises it.  Is it worth adding
   tNNNN-run-string-list-test.sh for some value of NNNN?  

I tend to agree with your second point and have a moderately negative
feeling against the patch.  An addition to API documentation like you
suggested would be more useful for people, and what Thiago did perhaps
might be useful in its sample code section, but then we already have
plenty of working samples in live code and they are single "git grep"
away, so...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]