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

From: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
Date: Aug 26, 2009, 03:54 UTC
Message-ID: <20090826035401.GJ3526@vidovic>
In-Reply-To: <7vvdkbl4ul.fsf@alter.siamese.dyndns.org>

The 25/08/09, Junio C Hamano wrote:

Show 9 quoted lines

> 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.

Ok, thank you.

> 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.

Could find it at the end of the mails.

Show 7 quoted lines

> >  #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?

It is for the warning() call

  warning("scissors line found, will skip text above");

I've added. That said, moving this declaration to builtin.h could be a good idea. Hint?

Show 7 quoted lines

> > @@ -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.

Good eyes.

Show 8 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.

Yes. Instead of looking for dashes alone, I will give a try to something like

	  if (!scissors_dashes_seen)
	    mark_start = i;
	  if (i + 1 < len) {
	    if (!memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2))) {
	      scissors_dashes_seen |= 02;
	      i++;
	      mark_end = i;
	      continue;
	    else if (!memcmp(buf + i "--", 2) {
	      scissors_dashes_seen |= 04;
	      i++;
	      mark_end = i;
	      continue;
	    }
	  }
	  if (i + 2 < len)
	    if (!memcmp(buf + i + 1, "- -", 3) {
	      scissors_dashes_seen |= 04;
	      i += 2;
	      mark_end = i;
	      continue;
	    }
	  if (buf[i] == '-') {
	    mark_end = i;
	    scissors_dashes_seen |= 01;
	    continue;
	  }
	  break;
	}
	
	if (scissors_dashes_seen == 07) {
	  ...

> it does not allow
> 
>     "-- 8< -- please cut here -- 8< -- --"

Actually, I believe this one should really not be a scissors line. If we accept some random dashes around markers it will break the definition of the mark itself.

As I said, I'd rather rules easy to define over others because if the end-user scissors line doesn't work, he can refer to the documentation...

Show 7 quoted lines

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

...and symmetrical markers make sense to the user. Will add this.

Show 11 quoted lines

> > +	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)) {
>         	...
> 	}

Does the compilers (or a standard) assure that the members are evaluated in the left-right order?

Otherwise, we may call is_scissors_line() where not needed.

-- 
Nicolas Sebrecht