Hi,
On Thu, 5 Jun 2008, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
