threads / discuss / 25373

format-patch on permission change gives empty patch

Subject: format-patch on permission change gives empty patch

## tl;dr

9 messages between Oct 7, 2010 and Oct 7, 2010.

replies: 8people: 5as markdown or json

David Miller· Oct 7, 2010, 00:37 UTC · lore

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
David Miller· Oct 7, 2010, 00:40 UTC · re: David Miller · lore

Re: format-patch on permission change gives empty patch

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?

Junio C Hamano· Oct 7, 2010, 04:13 UTC · re: David Miller · lore

Re: format-patch on permission change gives empty patch

David Miller <davem@davemloft.net> writes:
Show 13 quoted lines
> 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));
Junio C Hamano· Oct 7, 2010, 04:40 UTC · re: Junio C Hamano · lore

Re: format-patch on permission change gives empty patch

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.
Sverre Rabbelier· Oct 7, 2010, 08:15 UTC · re: Junio C Hamano · lore

Re: format-patch on permission change gives empty patch

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
Jakub Narebski· Oct 7, 2010, 10:05 UTC · re: Sverre Rabbelier · lore

Re: format-patch on permission change gives empty patch

Sverre Rabbelier <srabbelier@gmail.com> writes:
> On Thu, Oct 7, 2010 at 06:40, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> >  (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
Sverre Rabbelier· Oct 7, 2010, 10:12 UTC · re: Jakub Narebski · lore

Re: format-patch on permission change gives empty patch

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
Nicolas Pitre· Oct 7, 2010, 19:06 UTC · re: Junio C Hamano · lore

Re: format-patch on permission change gives empty patch

On Wed, 6 Oct 2010, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Junio C Hamano· Oct 7, 2010, 20:29 UTC · re: Nicolas Pitre · lore

Re: format-patch on permission change gives empty patch

Nicolas Pitre <nico@fluxnic.net> writes:
Show 9 quoted lines
> 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.

← back to recent threads