Re: format-patch on permission change gives empty patch

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2010, 04:40 UTC
Message-ID: <7vocb6y5sq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7vtykyy70w.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 20 quoted lines

> David Miller <davem@davemloft.net> writes:
>
>> From: David Miller <davem@davemloft.net>
>> Date: Wed, 06 Oct 2010 17:37:14 -0700 (PDT)
>>
>>> 
>>> When I ask git to format-patch a commit that is just a file
>>> permission change, it ends up generating an empty file, not
>>> even the commit message is included.
>>
>> Ok it turns out that the commit in question was a NOP since the file
>> permissions didn't change.
>>
>> But even if the patch is truly empty, format-patch should still give
>> me the commit message shouldn't it?
>
> Probably; we have strongly encouraged people not to commit no-op, so I
> guess nobody stumbled upon this corner case.
>
> Perhaps something like this?

Actually, I have a feeling that this is not merely a corner case we didn't care about.

A half-good news is that format-patch already takes --always command line option to generate a message out of an empty commit, but because it cannot be applied with "am", it is rather pointless.

BUT.

The weatherbaloon patch is probably a bad idea. "git rebase", especially when rebasing a side branch imported from some foreign SCM, would rather badly break with this patch, because its "format-patch | am" pipeline depends on format-patch to skip a no-op commit. Otherwise, "am" will complain about a patchless message. So in a sense, the current behaviour is internally consistent and deliberately so.

I have a mixed feeling about where to go next.

 (1) Treat "rebase" as a way to reproduce a reasonable history; the
     current behaviour to drop empty commits is consistent with this view,
     as a history with an empty commit is _not_ entirely reasonable.

 (2) Treat "rebase" as a way to reproduce history faithfully, even an
     unreasonable one.  We could teach "--allow-empty" to "am", and
     rewrite the pipeline as "format-patch --always | am --allow-empty" to
     implement it.

I think I would eventually end up doing the latter, but not tonight.