Re: [PATCH v3 1/2] Allow git-apply to ignore the hunk headers (AKA recountdiff)
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jun 6, 2008, 13:58 UTC
- Message-ID
- <alpine.DEB.1.00.0806061441120.1783@racer>
- In-Reply-To
- <7vve0nw4b7.fsf@gitster.siamese.dyndns.org>
Hi,
On Thu, 5 Jun 2008, Junio C Hamano wrote:
Show 8 quoted lines
> 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.
Show 6 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.
Maybe the easiest would be to set it to 1 regardless of the hunk.
Show 10 quoted lines
> > + 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.
Show 5 quoted lines
> > + 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.
Show 19 quoted lines
> > + 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.
Show 15 quoted lines
> > + 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.
Show 13 quoted lines
> 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