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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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?

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

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

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

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

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

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