Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2009, 01:51 UTC
Message-ID: <7vvdkbl4ul.fsf@alter.siamese.dyndns.org>
In-Reply-To: <fc2ecb5cf28cabb7d183e2835ce46aa9afb2a322.1251215299.git.nicolas.s.dev@gmx.fr>

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

Show 6 quoted lines

>> I think we have bikeshedded long enough, so I won't be touching this code
>> any further only to change the definition of what a scissors mark looks
>> like,
>
> I'm not sure I understand. Are you still open to a patch touching this code
> /too/?

What I meant was that I would not want to spend any more of _my_ time on the definition of the scissors for now. That means spending or wasting time on improving the 'pu' patch myself, or looking at others patch to find flaws in them.

Of course, as the maintainer, I would need to look at proposals to improve or fix bugs in the code before the series hits the master, but I would give zero priority to the patches that change the definition at least for now to give myself time to work on more useful things.

I think --ignore-scissors is a good thing to add, regardless of what the definition of scissors should be. So your patch should definitely be separated into two parts.

Show 9 quoted lines

> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index 7e09b51..92319f6 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -6,6 +6,7 @@
>  #include "builtin.h"
>  #include "utf8.h"
>  #include "strbuf.h"
> +#include "git-compat-util.h"

Inclusion of builtin.h is designed to be enough. What do you need this for?

Show 7 quoted lines

>  static FILE *cmitmsg, *patchfile, *fin, *fout;
>  
> @@ -25,6 +26,7 @@ static enum  {
>  static struct strbuf charset = STRBUF_INIT;
>  static int patch_lines;
>  static struct strbuf **p_hdr_data, **s_hdr_data;
> +static int ignore_scissors = 0;

Don't initialize a static to 0.

> @@ -715,51 +717,63 @@ static inline int patchbreak(const struct strbuf *line)
>  		if (isspace(buf[i])) {
> +			if (scissors_dashes_seen)
> +				mark_end = i;

I think you do not want this part, and then you won't have to trim trailing whitespaces from mark_end later.

Show 6 quoted lines

> +			/*
> +			 * The mark is 8 charaters long and contains at least one dash and
> +			 * either a ">8" or "<8". Check if the last mark in the line
> +			 * matches the first mark found without worrying about what could
> +			 * be between them. Only one mark in the whole line is permitted.
> +			 */

This definition makes "-            8<" a scissors.

Even though

    "-- 8< -- please cut here -- -- 8< --"

is allowed, so is

    "-- 8< -- -- please cut here -- 8< -- --"

it does not allow

    "-- 8< -- please cut here -- 8< -- --"

nor

    "-- 8< -- -- please cut here -- -- 8< --"

nor

    "-- 8< -- -- please cut here -- -- >8 --"

Oh, did I say I won't waste my time on the definition? I should have just discarded this hunk ;-)

Show 14 quoted lines

> @@ -782,22 +796,25 @@ static int handle_commit_msg(struct strbuf *line)
>  	if (metainfo_charset)
>  		convert_to_utf8(line, charset.buf);
>  
> -	if (is_scissors_line(line)) {
> -		int i;
> -		rewind(cmitmsg);
> -		ftruncate(fileno(cmitmsg), 0);
> -		still_looking = 1;
> +	if (!ignore_scissors) {
> +		if (is_scissors_line(line)) {
> +			warning("scissors line found, will skip text above");
> ...
> +			return 0;

Don't re-indent like this.  Just do:

	if (!ignore_scissors && is_scissors_line(line)) {
        	...
	}

> -	# -s, -u, -k, --whitespace, -3, -C, -q and -p flags are kept
> +	# Following flags are kept

We seem to have lost the description of what the "Following" are.