Subject: Re: [PATCH v4 1/7] quote: remove leading space in sq_dequote_step

From: Moritz Neeb <[email-masked]>
Date: Mon, 29 Feb 2016 22:45:56 +0100

On 02/29/2016 08:01 PM, Junio C Hamano wrote:
> Moritz Neeb <[email-masked]> writes:
> 
>> Because sq_quote_argv adds a leading space (which is expected in trace.c),
>> sq_dequote_step should remove this space again, such that the operations
>> of quoting and dequoting are inverse of each other.
>>
>> This patch is preparing the way to remove some excessive trimming
>> operation in bisect in the following commit.
>>
>> Signed-off-by: Moritz Neeb <[email-masked]>
>> ---
>>  quote.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/quote.c b/quote.c
>> index fe884d2..2714f27 100644
>> --- a/quote.c
>> +++ b/quote.c
>> @@ -63,6 +63,8 @@ static char *sq_dequote_step(char *arg, char **next)
>>  	char *src = arg;
>>  	char c;
>>  
>> +	if (*src == ' ')
>> +		src++;
>>  	if (*src != '\'')
>>  		return NULL;
>>  	for (;;) {
> 
> If we look at this "for (;;)" loop, we notice that (1) it accepts as
> many spaces as there are between two quoted strings, and (2) it does
> not limit it to SP but uses isspace().
> 
> I wonder if you would instead want
> 
> 	while (isspace(*src))
>         	src++;
> 
> to be consistent?
> 

My intention was to explicitly remove the space added by
strbuf_addch(dst, ' ') in sq_quote_argv().

I think it would not make sense to remove more spaces, because for
for sq_dequote() it is defined:

	This unwraps what sq_quote() produces in place, but returns
	NULL if the input does not look like what sq_quote would have
	produced.

I understand that this counts also for the sq_dequote_array*() family.

Thanks