Re: [PATCH v2] git-am: Ignore whitespace before patches

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 12, 2010, 20:04 UTC
Message-ID: <AANLkTikGaqj1QWtWEy3k=_E8q=bKNKjgJwHCQTUpnZmo@mail.gmail.com>
In-Reply-To: <AANLkTinDHvwHLZfj6DDPtV39Z2xhDZREiqwdt5cjiaLP@mail.gmail.com>

On Thu, Aug 12, 2010 at 19:13, Jay Soffian <jaysoffian@gmail.com> wrote:

Show 13 quoted lines

> On Wed, Aug 11, 2010 at 3:57 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>> Actually cut-and-paste is often a major source of whitespace breakage
>>> (including tabs silently being expanded), and I personally think a patch
>>> like this to encourage the practice is going in a wrong direction.
>>
>> I disagree and think git-am should be smarter. Any human looking at
>> something like a GMail mail.txt download will clearly see that it's a
>> patch, but git-am is pedantic and doesn't skip past whitespace at the
>> beginning of the file.
>
> The point of git-am being pedantic is to prevent the original patch
> from being applied w/silent corruption (e.g., tabs-to-spaces).

The git-am code doesn't strike me as particularly pedantic. It'll fail to spot any number of common pitfalls like patches being pasted inline (recently discussed on list), double encoding, invalid author names / E-Mail addresses etc. (which can e.g. happen when applying patches from RT).

The parsing code just didn't think of this issue, I'm not aware of any corruption that begins with whitespace being added to the beginning of a patch, but I am aware of a non-corruption (GMail) that does that.

> Perhaps, before making git-am less strict

I don't think it's less strict with this patch, just more intelligent.

> we should modify format-patch to include a sha1 of the diff output
> so that corruption can be reliably detected by git-am.

There's a lot we could do in this department, and there was a previous discussion on list about schemas like that (can't find it now).

We could do an ad-hoc checksum, but including more than the SHA would be better, e.g.:

    --
    cs:<7 char SHA1> t:<NUM TABS> s:<NUM SPACES> c:<NUM CHARS ([^
\t])> ln:<NUM LINES>

That'd allow git-am to print more intelligent error messages than just "ok/not ok", e.g.:

    * "your patch is $x lines, but the patch thinks it's $y, something
       may have gone wrong with wrapping"

    * "You have 0 tabs, but the patch thinks it has 20"

etc., but that's a project for another day.