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

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Ah, I personally think that is a crazy calling convention but the
function's loop consistently uses the "line at the beginning of iteration
points somewhere in the previous line to be skipped", so it would "work".
I was fooled by that.

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

I do not think line number information should be discarded.  If you have
two blocks of lines that look alike, the line number information does get
used to see which place the hunk should apply.

Deleting the common context lines from the beginning, or adding a new
"+added line" at the very beginning of a hunk, is a user error for
somebody who edits the diff.  Because you are not calling apply with
unidiff-zero, the sanity check applies to such a hunk.  Working around the
sanity check by discarding the line number information to make the patch
application even more error prone is an unacceptable hackery.

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

And that is even worse, and I thought you knew a lot better than that.
Sigh...

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

I explained what I wanted to _happen_ in a separate message.  Now to _how_
you would make it happen...

The way you use the return value from here is to cause parse_fragment() to
say "The patch is corrupt".  You do _not_ detect that here.  You are only
counting number of preimage and postimage lines in this function.  If the
next line does not look like a part of the current hunk, you stop counting
(iow, the only side effect you cause is to update oldlines and newlines in
fragment structure) without including that non-patch line, and return.
You let the caller to decide what that next line you excluded from the
current hunk is, because the caller _already_ has logic to decide what is
part of the patch text (it knows not just how hunk meat looks like but
also how hunk headers and "diff " to start the next patch looks like).
You do not want that information or logic here.

So the answer to "anal or lax" is "Neither.  It's none of your business".

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

Neither.  This calling site should not even decide.  The only thing
recount will tell its caller parse_fragment() is "I've recounted the
lines, so by iterating that many lines you will reach the end of the
current hunk as I determined.  Decide what the line beyond that is _your_
business, not mine".