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

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

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.

quoted

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

quoted

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

quoted

+			/*
+			 * 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...

nor

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

nor

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

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

quoted

+	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