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

Hi,

On Thu, 5 Jun 2008, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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

No.  At this point, it points directly after the last "@@", but still on 
the same line.

quoted

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

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

Because the old information should be ignored.  If the first line is a "+" 
line, the line number needs to be set to 1, otherwise the patch will not 
apply.

Maybe the easiest would be to set it to 1 regardless of the hunk.

quoted

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

No, as I said, the "line" parameter points just after "@@" of the hunk 
header.

BTW this is what I referred to when I said that it now works by design; 
formerly, it relied on a space being present after the "@@", which it 
would mistakenly count as a common line, which was the reason I set the 
counts to -1 initially.

quoted

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

No, see above.

quoted

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

No.  Because I can never know if the _positions_ in the hunk header are 
right.

quoted

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

Then I understood you not correctly at all when you complained about the 
"Probably a diff" part.

So what do you want?  Should it be anal, or lax?  You can't have both.

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.

quoted

@@ -1013,6 +1058,9 @@ static int parse_fragment(char *line, unsigned long size,
 	offset = parse_fragment_header(line, len, fragment);
 	if (offset < 0)
 		return -1;
+	if (offset > 0 && patch->recount &&
+			recount_diff(line + offset, size - offset, fragment))
+		return -1;

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

Again.  Lax or not lax?

Ciao,
Dscho