Re: [PATCH v3 1/2] Allow git-apply to ignore the hunk headers (AKA recountdiff)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 6, 2008, 16:13 UTC
- Message-ID
- <7vr6bav8ww.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <alpine.DEB.1.00.0806061441120.1783@racer>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
>> 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.
Show 10 quoted lines
>> > + 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".
Show 7 quoted lines
>> > + 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".