Re: [PATCH 1/2] rev list add option accepting revision constraints on standard input

[Andy Whitcroft <apw@shadowen.org>]

Junio C Hamano wrote:
> Andy Whitcroft <apw@shadowen.org> writes:
> 
>> Add a --stdin flag which causes rev-list to additionally read
>> its stdin stream and parse that for revision constraints.
> 
>> +/*
>> + * Parse revision information, filling in the "rev_info" structure,
>> + * revisions are taken from stream.
>> + */
>> +static void setup_revisions_stream(FILE *stream, struct rev_info *revs)
>> +{
>> +	char line[1000];
>> +	const char *args[] = { 0, line, 0 };
>> +
>> +	while (fgets(line, sizeof(line), stream) != NULL) {
>> +		line[strlen(line) - 1] = 0;
>> +
>> +		if (line[0] == '-')
>> +			die("options not supported in --stdin mode");
>> +
>> +		(void)setup_revisions(2, args, revs, NULL);
>> +	}
>> +}
> 
> Is calling setup_revisions() on the same revs like this many
> times safe?  I do not think so, especially what is after the
> primary "for()" loop in the function.
> 
> I was sort-of expecting that you would instead replace that
> primary for() loop in setup_revisions() with some sort of
> callback...

Heh, well I'll give it another poke tommorrow my time ...

Thanks for the feedback.  Its hard to get any context on something new
without trying and failing :).

-apw