Re: format-patch on permission change gives empty patch

Junio C Hamano [off-list ref] writes:

David Miller [off-list ref] writes:

quoted

From: David Miller <davem@davemloft.net>
Date: Wed, 06 Oct 2010 17:37:14 -0700 (PDT)

quoted

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.

Heya,

On Thu, Oct 7, 2010 at 06:40, Junio C Hamano [off-list ref] wrote:

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

Wouldn't we then have to keep adding options for other corner cases?
Perhaps a '--plumbing' flag that makes format-patch behave sanely
(e.g., also do stuff like, turn off color and whatnot) and that makes
'git am' accept everything it knows how to understand?

-- 
Cheers,

Sverre Rabbelier

Sverre Rabbelier [off-list ref] writes:

On Thu, Oct 7, 2010 at 06:40, Junio C Hamano [off-list ref] wrote:

quoted

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

Wouldn't we then have to keep adding options for other corner cases?
Perhaps a '--plumbing' flag that makes format-patch behave sanely
(e.g., also do stuff like, turn off color and whatnot) and that makes
'git am' accept everything it knows how to understand?

Hmmm... doesn't rebase pipeline use "format-patch --rebasing" (where
'--rebasing' is internal option deliberately left undocumented)?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Heya,

On Thu, Oct 7, 2010 at 12:05, Jakub Narebski [off-list ref] wrote:

Hmmm... doesn't rebase pipeline use "format-patch --rebasing" (where
'--rebasing' is internal option deliberately left undocumented)?

If so, that's exactly what I mean :)

-- 
Cheers,

Sverre Rabbelier

On Wed, 6 Oct 2010, Junio C Hamano wrote:

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.

But a file mode change isn't exactly an empty commit, no?


Nicolas