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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2010, 00:15 UTC
Message-ID: <7vbpg060qx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <6672d0161002071444lba0f751w3e7e33043e1ec2e8@mail.gmail.com>

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

Show 22 quoted lines

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