Re: [PATCH v3 1/2] Allow git-apply to ignore the hunk headers (AKA recountdiff)

Johannes Schindelin [off-list ref] writes:

+static int recount_diff(char *line, int size, struct fragment *fragment)
+{
+	int line_nr = 0;

At this point, line points at the beginning of the line that immediately
follows "@@ -oldpos,oldlines +newpos,newlines @@ ...\n", right?

+	if (size < 1)
+		return -1;
+
+	fragment->oldpos = 2;

Why do you discard oldpos information, and use magic number "2"?

+	fragment->oldlines = fragment->newlines = 0;
+
+	for (;;) {
+		int len = linelen(line, size);
+		size -= len;
+		line += len;

And you look at the line of the patch, measure how long it is, and you
already advance line to point at the next line without ever looking at the
contents of the line (you could look at line[-len], but that is crazy).

+		if (size < 1)
+			return 0;

Why?  It may be the last line in the hunk but you haven't done anything to
the current line yet.

+		switch (*line) {
+		case ' ': case '\n':
+			fragment->newlines++;
+			/* fall through */
+		case '-':
+			fragment->oldlines++;
+			break;
+		case '+':
+			fragment->newlines++;
+			if (line_nr == 0) {
+				fragment->leading = 1;
+				fragment->oldpos = 1;
+			}
+			fragment->trailing = 1;

Again, why muck with oldpos?  Also leading and trailing?  After you
recount the newlines and oldlines you ignored from the hunk header, the
caller will behave as if the original patch had the right numbers on the
hunk header to compute leading and trailing, doesn't it?

+			break;
+		case '@':
+			return size < 3 || prefixcmp(line, "@@ ");
+		case 'd':
+			return size < 5 || prefixcmp(line, "diff ");
+		default:
+			return -1;

I do not understand these return values.  Your caller, parse_fragment()
with your patch, gives you chance to recount the old and new line count,
and you are responsible for only recounting them.  The change you made to
parse_fragment() returns error, aborting the whole git-apply, when
recount_diff() returns non-zero, but having extra lines after the patch
text is perfectly fine and there is no reason to force aborting from
here.  IOW, this function should not even have power to do so.  The one
and only thing this function should do well is to reliably count the
number of patch lines.

... and this "return -1" is uncalled for.