format-patch on permission change gives empty patch

format-patch on permission change gives empty patch

[David Miller]

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.

davem@sunset:~/src/GIT/net-2.6$ git show
commit a5dbc62ed61bf4cc57e22b78e5794880f1c74b90
Author: Joe Perches <joe@perches.com>
Date:   Wed Oct 6 17:32:49 2010 -0700

    Documentation/networking/ixgbevf.txt: Change file permissions to 644
    
    Signed-off-by: Joe Perches <joe@perches.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
You have new mail in /var/mail/davem
davem@sunset:~/src/GIT/net-2.6$ git format-patch HEAD^
0001-Documentation-networking-ixgbevf.txt-Change-file-per.patch
davem@sunset:~/src/GIT/net-2.6$ ls -l 0001-Documentation-networking-ixgbevf.txt-Change-file-per.patch 
-rw-r--r-- 1 davem davem 0 Oct  6 17:36 0001-Documentation-networking-ixgbevf.txt-Change-file-per.patch
davem@sunset:~/src/GIT/net-2.6$ git version
git version 1.7.3.1

Re: format-patch on permission change gives empty patch

[David Miller]

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?

Re: format-patch on permission change gives empty patch

[Junio C Hamano]

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?

 builtin/log.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 22d1290..6baba7d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1057,6 +1057,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.verbose_header = 1;
 	rev.diff = 1;
 	rev.no_merges = 1;
+	rev.always_show_header = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));

Re: format-patch on permission change gives empty patch

[Junio C Hamano]

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

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

Re: format-patch on permission change gives empty patch

[Sverre Rabbelier]

Heya,

On Thu, Oct 7, 2010 at 06:40, Junio C Hamano <gitster@pobox.com> 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

Re: format-patch on permission change gives empty patch

[Jakub Narebski]

Sverre Rabbelier <srabbelier@gmail.com> writes:
> On Thu, Oct 7, 2010 at 06:40, Junio C Hamano <gitster@pobox.com> 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?

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

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: format-patch on permission change gives empty patch

[Sverre Rabbelier]

Heya,

On Thu, Oct 7, 2010 at 12:05, Jakub Narebski <jnareb@gmail.com> 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

Re: format-patch on permission change gives empty patch

[Nicolas Pitre]

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

Re: format-patch on permission change gives empty patch

[Junio C Hamano]

Nicolas Pitre <nico@fluxnic.net> writes:

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

The second message from DaveM:

    Message-ID: <20101006.174008.70175671.davem@davemloft.net>

    Ok it turns out that the commit in question was a NOP since the file
    permissions didn't change.