Bug: Incorrect stripping of the [PATCH] prefix in git-am

Bug: Incorrect stripping of the [PATCH] prefix in git-am

[huebbe]

[-- Attachment #1: Type: text/plain, Size: 2041 bytes --]

Description:

When applying a patch created via `git format-patch` with `git am`,
any prefix of the commit message that's within square brackets is stripped from the commit message.


Reproduction:

    $ git log --oneline --decorate --graph --all
    * b41514b (HEAD) [baz] baz
    | * 5e31740 (master) [bar] bar
    |/
    * aaf5d34 [foo] foo
    $ git format-patch aaf5d34
    $ git checkout master
    $ git am 0001-baz-baz.patch
    $ git log --oneline --decorate --graph --all
    * d5161b8 (HEAD, master) baz
    * 5e31740 [bar] bar
    * aaf5d34 [foo] foo

I have omitted all output except for the `git log` output for brevity.
As you can see, the commit resulting from `git am` has lost the "[bar]" prefix from its commit message.

Looking at the patch,

    $ cat 0001-baz-baz.patch
    From b41514be8a70fd44052bff0d3ce720373ec9cfd8 Mon Sep 17 00:00:00 2001
    From: Nathanael Huebbe <nathanael.huebbe@informatik.uni-hamburg.de>
    Date: Wed, 25 Nov 2015 15:28:09 +0100
    Subject: [PATCH] [baz] baz

    ---
     baz | 1 +
     1 file changed, 1 insertion(+)
     create mode 100644 baz

    diff --git a/baz b/baz
    new file mode 100644
    index 0000000..7601807
    --- /dev/null
    +++ b/baz
    @@ -0,0 +1 @@
    +baz
    --
    2.1.4

I see that the commit message contains the "[PATCH]"-prefix that `git am` is supposed to strip,
yet it seems to incorrectly continue to also strip the "[baz]"-prefix.


Affected versions:
I have reproduced the bug with versions 1.9.1, 2.1.4, and 2.6.3


Severity:
While I do not consider this a high priority bug, it becomes quite irksome in some workflows.
In my case, an upstream `svn` repository has the policy of using "[branch-name]" prefixes
to the commit messages, which are stripped whenever I transplant a commit using the
`git format-patch`/`git am` combination.



-- 
Please be aware that the enemies of your civil rights and your freedom
are on CC of all unencrypted communication. Protect yourself.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[stefan.naewe]

Am 25.11.2015 um 16:29 schrieb huebbe:
> Description:
> 
> When applying a patch created via `git format-patch` with `git am`,
> any prefix of the commit message that's within square brackets is stripped from the commit message.

As advertised in the man page of 'git am' (or 'git mailinfo' that is).

Is 'git am --keep-non-patch' what you're looking for ?

> 
> 
> Reproduction:
> 
>     $ git log --oneline --decorate --graph --all
>     * b41514b (HEAD) [baz] baz
>     | * 5e31740 (master) [bar] bar
>     |/
>     * aaf5d34 [foo] foo
>     $ git format-patch aaf5d34
>     $ git checkout master
>     $ git am 0001-baz-baz.patch
>     $ git log --oneline --decorate --graph --all
>     * d5161b8 (HEAD, master) baz
>     * 5e31740 [bar] bar
>     * aaf5d34 [foo] foo
> 
> I have omitted all output except for the `git log` output for brevity.
> As you can see, the commit resulting from `git am` has lost the "[bar]" prefix from its commit message.
> 
> Looking at the patch,
> 
>     $ cat 0001-baz-baz.patch
>     From b41514be8a70fd44052bff0d3ce720373ec9cfd8 Mon Sep 17 00:00:00 2001
>     From: Nathanael Huebbe <nathanael.huebbe@informatik.uni-hamburg.de>
>     Date: Wed, 25 Nov 2015 15:28:09 +0100
>     Subject: [PATCH] [baz] baz
> 
>     ---
>      baz | 1 +
>      1 file changed, 1 insertion(+)
>      create mode 100644 baz
> 
>     diff --git a/baz b/baz
>     new file mode 100644
>     index 0000000..7601807
>     --- /dev/null
>     +++ b/baz
>     @@ -0,0 +1 @@
>     +baz
>     --
>     2.1.4
> 
> I see that the commit message contains the "[PATCH]"-prefix that `git am` is supposed to strip,
> yet it seems to incorrectly continue to also strip the "[baz]"-prefix.
> 
> 
> Affected versions:
> I have reproduced the bug with versions 1.9.1, 2.1.4, and 2.6.3
> 
> 
> Severity:
> While I do not consider this a high priority bug, it becomes quite irksome in some workflows.
> In my case, an upstream `svn` repository has the policy of using "[branch-name]" prefixes
> to the commit messages, which are stripped whenever I transplant a commit using the
> `git format-patch`/`git am` combination.
> 
> 
> 

HTH,
  Stefan
-- 
----------------------------------------------------------------
/dev/random says: Oxymoron: Slow speed.
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')" 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[huebbe]

[-- Attachment #1: Type: text/plain, Size: 2796 bytes --]

Yes, it looks like the `--keep-non-patch` option works around this.

However, shouldn't that be the default behaviour?
I mean, what is the point in stripping stuff that is not proven to be inserted by `git` itself?
That's not what I expect a tool to do which I trust.

Cheers,
Nathanael



On 11/25/2015 04:44 PM, stefan.naewe@atlas-elektronik.com wrote:
> Am 25.11.2015 um 16:29 schrieb huebbe:
>> Description:
>>
>> When applying a patch created via `git format-patch` with `git am`,
>> any prefix of the commit message that's within square brackets is stripped from the commit message.
> 
> As advertised in the man page of 'git am' (or 'git mailinfo' that is).
> 
> Is 'git am --keep-non-patch' what you're looking for ?
> 
>>
>>
>> Reproduction:
>>
>>     $ git log --oneline --decorate --graph --all
>>     * b41514b (HEAD) [baz] baz
>>     | * 5e31740 (master) [bar] bar
>>     |/
>>     * aaf5d34 [foo] foo
>>     $ git format-patch aaf5d34
>>     $ git checkout master
>>     $ git am 0001-baz-baz.patch
>>     $ git log --oneline --decorate --graph --all
>>     * d5161b8 (HEAD, master) baz
>>     * 5e31740 [bar] bar
>>     * aaf5d34 [foo] foo
>>
>> I have omitted all output except for the `git log` output for brevity.
>> As you can see, the commit resulting from `git am` has lost the "[bar]" prefix from its commit message.
>>
>> Looking at the patch,
>>
>>     $ cat 0001-baz-baz.patch
>>     From b41514be8a70fd44052bff0d3ce720373ec9cfd8 Mon Sep 17 00:00:00 2001
>>     From: Nathanael Huebbe <nathanael.huebbe@informatik.uni-hamburg.de>
>>     Date: Wed, 25 Nov 2015 15:28:09 +0100
>>     Subject: [PATCH] [baz] baz
>>
>>     ---
>>      baz | 1 +
>>      1 file changed, 1 insertion(+)
>>      create mode 100644 baz
>>
>>     diff --git a/baz b/baz
>>     new file mode 100644
>>     index 0000000..7601807
>>     --- /dev/null
>>     +++ b/baz
>>     @@ -0,0 +1 @@
>>     +baz
>>     --
>>     2.1.4
>>
>> I see that the commit message contains the "[PATCH]"-prefix that `git am` is supposed to strip,
>> yet it seems to incorrectly continue to also strip the "[baz]"-prefix.
>>
>>
>> Affected versions:
>> I have reproduced the bug with versions 1.9.1, 2.1.4, and 2.6.3
>>
>>
>> Severity:
>> While I do not consider this a high priority bug, it becomes quite irksome in some workflows.
>> In my case, an upstream `svn` repository has the policy of using "[branch-name]" prefixes
>> to the commit messages, which are stripped whenever I transplant a commit using the
>> `git format-patch`/`git am` combination.
>>
>>
>>
> 
> HTH,
>   Stefan
> 


-- 
Please be aware that the enemies of your civil rights and your freedom
are on CC of all unencrypted communication. Protect yourself.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[Jeff King]

On Wed, Nov 25, 2015 at 04:59:35PM +0100, huebbe wrote:

> Yes, it looks like the `--keep-non-patch` option works around this.
> 
> However, shouldn't that be the default behaviour?
> I mean, what is the point in stripping stuff that is not proven to be inserted by `git` itself?
> That's not what I expect a tool to do which I trust.

The "[]" convention is a microformat used by Linux kernel folks. So it's
not "whoops, we are stripping stuff not added by git". It is respecting
a microformat used by the tool's authors.

That being said, if we were choosing a default from scratch today, it
might go the other way. But we aren't, and we have to deal with the
burden of breaking existing scripts by flipping it.

-Peff

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[Stefan Beller]

On Tue, Dec 1, 2015 at 4:58 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 25, 2015 at 04:59:35PM +0100, huebbe wrote:
>
>> Yes, it looks like the `--keep-non-patch` option works around this.
>>
>> However, shouldn't that be the default behaviour?
>> I mean, what is the point in stripping stuff that is not proven to be inserted by `git` itself?
>> That's not what I expect a tool to do which I trust.
>
> The "[]" convention is a microformat used by Linux kernel folks. So it's
> not "whoops, we are stripping stuff not added by git". It is respecting
> a microformat used by the tool's authors.
>
> That being said, if we were choosing a default from scratch today, it
> might go the other way. But we aren't, and we have to deal with the
> burden of breaking existing scripts by flipping it.

Do we as the Git community have a place where we take notes for version 3?

This sounds perfectly fine (to me at least) to change the default with a major
version number as the changelog for major version numbers may even be read
by kernel devs maintaining such scripts ;)

Or could we even patch that today to have `--keep-non-patch` be default
later on by checking for our version number ourselves and deciding upon that
for the default?

>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Do we as the Git community have a place where we take notes for version 3?

I do not think there is, I do think we might need one when a need
arises, and I do not think this topic is one that creates such a
need.

And I said "might" in the second one, based on the experience at
2.0; we didn't need such a separate place when managing the
backward incompatible changes for that release.

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The "[]" convention is a microformat used by Linux kernel folks. So it's
> not "whoops, we are stripping stuff not added by git". It is respecting
> a microformat used by the tool's authors.
>
> That being said, if we were choosing a default from scratch today, it
> might go the other way. But we aren't, and we have to deal with the
> burden of breaking existing scripts by flipping it.

And I do think it no longer is sensible to expect that it still is
kernel-only convention.  Any project that uses e-mail based workflow
with Git have known how "[]" microformat works, may even have taken
advantage of it to build their workflow around it, and flipping the
default will only hurt them.

A project that chooses not to follow the convention can easily tweak
a knob to keep using different conventions, so I do not see anything
to change here.

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[huebbe]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

On 12/02/2015 01:58 AM, Jeff King wrote:
> On Wed, Nov 25, 2015 at 04:59:35PM +0100, huebbe wrote:
> 
>> Yes, it looks like the `--keep-non-patch` option works around this.
>>
>> However, shouldn't that be the default behaviour?
>> I mean, what is the point in stripping stuff that is not proven to be inserted by `git` itself?
>> That's not what I expect a tool to do which I trust.
> 
> The "[]" convention is a microformat used by Linux kernel folks. So it's
> not "whoops, we are stripping stuff not added by git". It is respecting
> a microformat used by the tool's authors.
> 
> That being said, if we were choosing a default from scratch today, it
> might go the other way. But we aren't, and we have to deal with the
> burden of breaking existing scripts by flipping it.
> 
> -Peff

Ok, I think I understand the reason for this behavior now.
However, I still believe that the `git format-patch`/`git am` combo
should retain the original commit messages by default.

As such, I would like to ask whether it would be possible/sensible
to somehow escape square brackets, or mark the beginning
of the original commit message in the `git format-patch` output?
This would allow `git am` to reproduce the exact commit message by default
without breaking the "[]" convention.


For example, I could imagine to use this behavior:

  * `git format-patch` produces a message of the format "[PATCH ...]: original commit message".

  * `git am` strips the longest prefix that consists entirely of bracketed stuff
    and ends with the string "]: ". If that terminator is not found -> old behavior.

  * Users could insert any "[FOO]" strings they like before the ":",
    which would still be stripped from the commit message by `git am`.

Of course, any such change would not be without pitfalls. With the rule above,
we would get ": " prepended to commit messages if a patch is created with a `git` version
that has the change, and applied with an older version that does not know about the terminator.


What do you guys think? Is there anything I missed?


Cheers,
Nathanael


-- 
Please be aware that the enemies of your civil rights and your freedom
are on CC of all unencrypted communication. Protect yourself.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[Jeff King]

On Wed, Dec 02, 2015 at 01:38:18PM +0100, huebbe wrote:

> As such, I would like to ask whether it would be possible/sensible
> to somehow escape square brackets, or mark the beginning
> of the original commit message in the `git format-patch` output?
> This would allow `git am` to reproduce the exact commit message by default
> without breaking the "[]" convention.

I am not sure why "git format-patch -k | git am -k" does not do what you
want. That is what those options were added for (and what git-rebase
uses internally to make sure commit messages are left unmunged).

-Peff

Re: Bug: Incorrect stripping of the [PATCH] prefix in git-am

[huebbe]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

Dear Peff,
I have no problem working around this bug/feature.

I just happen to think that the current *default* behaviour
is not the default behaviour that users have a right to expect:
I believe that users have every right to expect `git format-patch`/`git am`
to preserve commit messages perfectly by default.

Since I see that people are using the current behaviour as a feature,
I tried to come up with an alternative behaviour that would do both:

 1. Respect user assumptions to preserve commit messages by default.

 2. Allow users to prepend stuff when mailing a patch, that will get stripped automatically.

Just because the current behaviour is ... irritating.


Cheers,
Nathanael



On 12/02/2015 04:49 PM, Jeff King wrote:
> On Wed, Dec 02, 2015 at 01:38:18PM +0100, huebbe wrote:
> 
>> As such, I would like to ask whether it would be possible/sensible
>> to somehow escape square brackets, or mark the beginning
>> of the original commit message in the `git format-patch` output?
>> This would allow `git am` to reproduce the exact commit message by default
>> without breaking the "[]" convention.
> 
> I am not sure why "git format-patch -k | git am -k" does not do what you
> want. That is what those options were added for (and what git-rebase
> uses internally to make sure commit messages are left unmunged).
> 
> -Peff
> 


-- 
Please be aware that the enemies of your civil rights and your freedom
are on CC of all unencrypted communication. Protect yourself.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]