Re: [PATCH/RFC 1/2] fetch-pack: new option to read refs from stdin

From: Ivan Todoroski <grnch_lists@gmx.net>
Date: Mar 25, 2012, 09:39 UTC
Message-ID: <4F6EE7E2.1020702@gmx.net>
In-Reply-To: <20120325011948.GC27651@sigill.intra.peff.net>

On 25.03.2012 03:19, Jeff King wrote:

Show 11 quoted lines

> On Sat, Mar 24, 2012 at 09:53:26PM +0100, Ivan Todoroski wrote:
> 
>> ---
>>  Documentation/git-fetch-pack.txt |    9 ++++++++
>>  builtin/fetch-pack.c             |   44 ++++++++++++++++++++++++++++++++++++--
>>  fetch-pack.h                     |    3 ++-
>>  3 files changed, 53 insertions(+), 3 deletions(-)
> 
> Give more of a commit message. Why is this option useful (I know, of
> course, from our previous discussion. But keep in mind the audience of
> developers reading "git log" a year from now).

Definitely. This was just work in progress I posted while I figure out the test suite problem. I will add full commit messages when I complete the test cases. Thanks for reviewing, I will incorporate all your suggestions in the next version.

Show 13 quoted lines

>> @@ -972,6 +976,42 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
>>  	if (!dest)
>>  		usage(fetch_pack_usage);
>>  
>> +	if (args.refs_from_stdin) {
>> +		char ref[1000];
> 
> Ick. Is there any reason not to use a strbuf here? 1000 is probably
> plenty, but we are generally moving towards removing such limits where
> possible.
> 
> You'd also get to use strbuf_getline and strbuf_trim in the
> newline-delimited case.

Right now that "char ref[1000]" code is rejecting refs on stdin if they are longer than 1000 chars or if they contain an ASCII NUL char in them. When I change this to strbuf_getline() should I be doing any similar checks, or do I just pass on whatever I read?