Re: [PATCH] Add a test for a problem in "rebase --whitespace=fix"

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

Björn Gustavsson <bgustavsson@gmail.com> writes:

> 2010/2/7 Junio C Hamano <gitster@pobox.com>:
>
>> First, is this a condition that we want to change the behaviour to
>> "succeed" later?
>
> Yes, assuming it is possible to fix.
>
>> Imagine that the gap between abc and def block in your example is much
>> larger to exceed the number of pre-context lines of your second patch
>> (usually 3), and imagine you are the "git apply --whitespace=fix" program
>> you have updated to "fix" the preceived problem.  You know you earlier
>> might have stripped some blank lines at the EOF, but there is nothing that
>> tells you if you had only 3 blank lines, or you had even more.  How many
>> blank lines will you be adding back?
>
> My original idea was to add back exactly the number of lines needed
> so that the context lines would match. That can be calculated from
> the line numbers of the last line of the pre-image and the line number
> in the chunk and by scanning the chunk for blank context lines
> (both at the beginning and end of chunk). Since the blanks lines
> at the end will be stripped away anyway, I doesn't matter if I add
> back fewer lines than were there originally.

And if it were in the middle like your patch had?  

Suppose the first patch in your example ended with 10 blank lines instead
of just one.  You apply it with --ws=fix and end up with 3-liner file with
a/b/c.  The sender of that patch then builds on top of his copy (still
with 10 blanks at EOF).  Perhaps the early part a/b/c might be changed to
a/b/c/1/2 or something.  And on top of that change, he adds new text at
the end of the file (after those 10 blank lines) with another patch, like
your example added d/e/f at the end after the gap.

The sender then chooses to cherry pick and gives you only the last patch,
to add d/e/f, for whatever reason.  The change in the earlier part to add
1/2 after a/b/c was not suitable for public consumption yet, perhaps.  The
patch comes with 3 pre-context lines as usual, what you see begins with
three blank lines, and has an addition of d/e/f.

You have already stripped the blank lines at the end when you applied the
original one; you do not know how many blank lines at the end you lost
when you applied it (and you do not _want_ to record that when applying
either).

You cannot go by the line numbers on the "@@ -l,k +m,n @@" header line you
see in the second patch you received.  On that line, only k and n are
reliable numbers (the must match the patch text).  l and m are unreliable;
being able to apply even if the text you have at hand does not exactly
match l and m is the whole point of transmitting the change in the patch
format.  The _only_ information you have usable at that point is that
there are _at least_ 3 blank lines before the addition, and perhaps the
fact that the hunk ends without post context lines.  The latter tells you
that it must apply at the end, but still doesn't tell you how many blanks
you need to add back at EOF before applying the patch.

> Do you see any fatal flaws that I don't see?

I don't know---the above is what I already said in my previous message and
it already looked fatal enough to me.