From: SZEDER Gábor <szeder@ira.uka.de>
Date: Mon, 08 Jun 2015 16:30:19 GMT
Subject: Re: [PATCH] commit: cope with scissors lines in commit message
Message-ID: <20150608183019.Horde.Kp4b2pClPLqAJp3LacT_fA1@webmail.informatik.kit.edu>
In-Reply-To: <xmqqoakqciv1.fsf@gitster.dls.corp.google.com>


Quoting Junio C Hamano <gitster@pobox.com>:

> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> The diff and submodule shortlog appended to the commit message template
>> by 'git commit --verbose' are not stripped when the commit message
>> contains an indented scissors line.
>>
>> When cleaning up a commit message with 'git commit --verbose' or
>> '--cleanup=scissors' the code is careful and triggers only on a pure
>> scissors line, i.e. a line containing nothing but a comment character, a
>> space, and the scissors cut.  This is good, because people can embed
>> scissor lines in the commit message while using 'git commit --verbose',
>> and the text they write after their indented scissors line doesn't get
>> deleted.
>>
>> While doing so, however, the cleanup function only looks at the first
>> line matching the scissors pattern and if it doesn't start at the
>> beginning of the line, then the function just returns without performing
>> any cleanup.  This is bad, because a "real" scissors line added by 'git
>> commit --verbose' might follow, and in that case the diff and submodule
>> shortlog get included in the commit message.
>
> Yikes; this is not just "bad" but simply "wrong".  Thanks for
> noticing.

Great, the other day I scored a "Gaaah" from Peff, now a "Yikes" from  
you...  Doin' good! ;)

>>  void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
>>  {
>> -	const char *p;
>> +	const char *p = buf->buf;
>>  	struct strbuf pattern = STRBUF_INIT;
>>
>>  	strbuf_addf(&pattern, "%c %s", comment_line_char, cut_line);
>> -	p = strstr(buf->buf, pattern.buf);
>> -	if (p && (p == buf->buf || p[-1] == '\n'))
>> -		strbuf_setlen(buf, p - buf->buf);
>> +	while ((p = strstr(p, pattern.buf))) {
>> +		if (p == buf->buf || p[-1] == '\n') {
>> +			strbuf_setlen(buf, p - buf->buf);
>> +			break;
>> +		}
>> +		p++;
>> +	}
>
> I however wonder if we should make strstr() do more work for us.
>
> 	strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
> 	if (starts_with(buf->buf, pattern.buf + 1))
> 		strbuf_setlen(buf, 0);
> 	else if ((p = strstr(buf->buf, pattern.buf)) != NULL)
>         	strbuf_setlen(buf, p - buf->buf + 1);
> 	strbuf_release(&pattern);
>
> perhaps?

Though this has one more if statement, it doesn't add a loop and  
eliminates the if (... || ...).
Good, will try to reroll in the evening.

Gábor