git-svn rebase screwing up commit messages

Hello, git version 1.5.2.3 over here, I've googled and searched in the ML archives but did not find this: when I git-svn rebase, my commits in Git (that are temporarily removed and then re-applied) get their commit message flattened on a single line (needless to say this is utterly annoying :D).

Is this a bug or something?

Thanks.

Benoit SIGOURE <tsuna@lrde.epita.fr> writes:

Do you mean by "my commits in Git" a commit you created with git in your git repository?

On Sat, 28 Jul 2007 05:14:26 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Hi,

Tested this here (rc3.24.g83b3d) and can confirm the reported problem. After making a commit in git and then running "git svn rebase" to receive updates from the svn repo, the rebased commit has a borked description (multi-lined commit message appears all on one line).

Sean

Sean <seanlkml@sympatico.ca> writes:

In short, your original commit log message is broken.

The recommended convention for commit messages is to start it with a single line that describes what it does, followed by a blank line (i.e. the first paragraph consists of a single line), followed by a longer explanation of why the change brought by the commit is a good thing.

Following this convention is recommended to make other peoples' lives more pleasant, and git assumes you follow this convention at several places. For example, "git log --pretty=oneline", "git reflog", and "git show-branch" are ways to get concise listing of commits; git-shortlog gives the list of such commit titles in its output, omitting the longer explanation. Patches prepared for e-mail exchange ("git format-patch", and --pretty=email) use the title on the Subject: line of the message.

The behaviour of these tools are _not_ the primarly reasons why you are better off following the commit message convention. The reason behind the behaviour of these tools is to help readers of your commit messages. The punch line comes at the beginning, single line short-and-sweet, and when readers want to get the birds-eye view, that is the only thing they see.

Try them on a commit that has several lines in its first paragraph and you will see why this is a good convention.

Until recently, we chomped a multi-line first paragraph at the first line, and used the resulting partial sentence as the title. This had an interesting effect on badly formatted commit like this:

        tree 2a003266ee6fda9305833a4f6e6dc7194018805a
        parent fef221460121c8a1b9e242422d8b521fbd0f6dc0
        author Junio C Hamano <gitster@pobox.com> 1185621830 -0700
        committer Junio C Hamano <gitster@pobox.com> 1185621830 -0700

        A long and unsightly commit
        log message that has more than
        one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

When this was formatted for e-mail, it resulted in something like this:

        From: Junio C Hamano <gitster@pobox.com>
        Date: Sat, 28 Jul 2007 04:23:50 -0700
        Subject: [PATCH] A long and unsightly commit
        log message that has more than
        one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

As rebase/am uses the same machinery as e-mailed patch acceptance, the paragraph was chomped at the first line, while normalizing it for other git commands' use, like this:

        tree 842c159f584fd4f970b6aceed21f479c1b62e333
        parent 57887443c24e5a2b4b04e7db69b44b53d8e87b44
        author Junio C Hamano <gitster@pobox.com> 1185621830 -0700
        committer Junio C Hamano <gitster@pobox.com> 1185626445 -0700

        A long and unsightly commit

        log message that has more than
        one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

This would mean that "oneline" format will see only the initial part of the sentence. If your message is properly formatted, it is not a problem.

Recent enough git instead uses RFC2822 line-folding like this, to help noncomforming messages somewhat:

        From 4c04a94...
        From: Junio C Hamano <gitster@pobox.com>
        Date: Sat, 28 Jul 2007 04:23:50 -0700
        Subject: [PATCH] A long and unsightly commit
         log message that has more than
         one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

But this has an interesting side effect itself. Such a folded line is logically treated as a single line, and rebase/am makes a commit like this out of such a message:

        tree 842c159f584fd4f970b6aceed21f479c1b62e333
        parent 57887443c24e5a2b4b04e7db69b44b53d8e87b44
        author Junio C Hamano <gitster@pobox.com> 1185621830 -0700
        committer Junio C Hamano <gitster@pobox.com> 1185626469 -0700

        A long and unsightly commit log message that has more than one lines in the first paragraph

        Such a first paragraph is flattened by --pretty=email

which is still unsightly (but the original message is unfriendly to oneline summary format to begin with). At least, this is an improvement in that the new formatting does not lose information when viewed in "oneline" format.

Having said all that, so that the readers understand the background, here is a not-so-heavily-tested patch, which might help. It passes all the test suite as before, but that tells how existing git-svn tests do not test many things.

I am not considering this for inclusion right now, by the way.

-- >8 -- Rebase/am: preserve multi-line first paragraph better.

This is actually three patches folded into one; as such it should not be applied as-is. It needs to be split into:

 * Changes to refs.c::log_ref_write() to sanitize embedded
   newlines from the log message, instead of chomping it at the
   first newline;

 * Changes to symbolic-ref and update-ref, so that they do not
   refuse a reflog message that has embedded newlines.  They
   have no business in dictating what the reflog message should
   look like.

 * Changes to builtin-mailinfo.c to preserve LF in Subject:
   header, when it is folded using RFC2822 header folding.

With this, the first paragraph of a commit message that has multiple lines is reproduced by "am" without being squashed into a single line.

---

 builtin-mailinfo.c     |   29 +++++++++++++++++++++++------
 builtin-symbolic-ref.c |    2 --
 builtin-update-ref.c   |    2 --
 refs.c                 |   39 +++++++++++++++++++++++----------------
 4 files changed, 46 insertions(+), 26 deletions(-)

On Sat, 28 Jul 2007 06:10:49 -0700 Junio C Hamano <gitster@pobox.com> wrote:

That explains why I hadn't seen the problem before, since I usually follow the commit message convention. For testing purposes I had simply mashed the home row to add five or six lines without thinking...

Yes, Bisecting shows this "problem" was introduced in your commit 4234a76167 which mentions that commit messages following the normal convention would be unaffected.

...[SNIPPED]...

FWIW your patch fixed my test case here.

Cheers, Sean

Sean <seanlkml@sympatico.ca> writes:

Actually the patched behaviour actively encourages a bad (not in the sense that those oneline tools will not work well, but in the sense that these messages are reader unfriendly) practice; I do not think what the patch did deserves to be called "fixed".

And that is one of the reasons, other than that we are in -rc freeze that we do not add anything but unarguable fixes, that I am not considering the patch for inclusion right now.

On Sat, 28 Jul 2007 06:38:19 -0700 Junio C Hamano <gitster@pobox.com> wrote:

First, i didn't read the patch and i have no stake at all in non-conforming commit messages; i always follow the convention. Having said that, the current behavior of Git crosses the line from advocating the common commit message format into the realm of not-working properly with non-conforming commit messages.

I would argue that you shouldn't try to have it both ways. Either Git supports non-conforming message formats, in which case the current behavior seems buggy. Or Git does not support commit messages that deviate from the standard, in which case the documentation should be updated to state so bluntly.

If handling email-mangled commit messages means that non-conforming formats can no longer be fully supported (while discouraged) then perhaps the time has come to explicitly state that people should not expect Git to handle anything but the 1+empty+description commit message format.

Sean

Sean <seanlkml@sympatico.ca> writes:

You cannot have it both ways as-is, but this is solvable. The invocation of am from rebase needs an extra (internal to implementation) option to use the code it patches, and the regular am can fold what are found on Subject: lines it used to.

The patch as-is breaks the more important case of e-mail
acceptance codepath, because MUAs are free to fold the Subject:
line when the original line is long, and what the user (the
original patch submitter) expects to happen is that a
single-line-ness of the original Subject: of the message to be
kept.  The patch breaks such a line at the place MUA happens to
fold such a long, single line, for comforming messages.
 

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

So here comes an updated series, properly split along the logical lines.

  0001-log_ref_write-do-not-chomp-reflog-message-at-the.txt
  0002-symbolic-ref-update-ref-do-not-refuse-reflog-message.txt
  0003-rebase-try-not-to-munge-commit-log-message.txt

I am not absolutely sure about the implications of mailinfo change. I tried to be careful by making sure that:

 - the updated code kicks in when "format-patch piped to am"
   toolchain uses the -k option on both ends, as rebase/am
   does.  This is the case where we would want to keep the
   way original commit log message was formatted.

 - otherwise the original "line folding" clean-up is used, so
   that usual e-mailed patch acceptance codepath cleanses the
   oneline summary obtained from the "Subject:" header.

Because it would be a serious regression to break the latter, while the former is just "an improvement" [*1], somebody really should double check the change for me.

[Footnote]

*1* It does not mean that improved support for foreign SCM interoperation does not matter. It is "merely an improvement" in the sense that people do not like what the code currently does anyway, and if the updated code is still not liked, then not having such "an improvement" does not matter.

A reflog file is organized as one-line-per-entry records, and we enforced the file format integrity by chomping the given message at the first LF. This changes it to convert them to SP, which is more in line with the --pretty=oneline format.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.c |   44 ++++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 16 deletions(-)

Hi,

On Sat, 28 Jul 2007, Junio C Hamano wrote:

Would it not be better to chop off before the first "\n", and just append "..."? IOW something like

-- snip --
 refs.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I've considered it, but decided against it because:

 (1) I did not like the information lossage;

 (2) this is solely to help log messages coming from outside the
     recommended convention, and having excess will not hurt the
     normal usage;

 (3) it is not known if messages from outside the recommended
     convention have enough information on its first line of the
     first paragraph; the result of s/\n.*/.../ may not be
     sufficient "hint";

 (4) log output are by default piped to "less -S" so extra
     output gives the user chance to inspect more if he chosses
     to, without hurting the end user display; and

 (5) the code was simpler and less error prone to go wrong.

Hi,

On Sun, 29 Jul 2007, Junio C Hamano wrote:

Good catch.  Didn't think of that.

Ciao, Dscho

Earlier these tools refused to create a reflog entry when the message given by the calling Porcelain had a LF in it, partially to keep the file format integrity of reflog file, which is one-entry-per-line. These tools should not be dictating such a policy.

Instead, let the codepath to write out the reflog entry worry about the format integrity and allow messages with LF in them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-symbolic-ref.c |    2 --
 builtin-update-ref.c   |    2 --
 2 files changed, 0 insertions(+), 4 deletions(-)

This makes rebase/am keep the original commit log message better, even when it does not conform to "single line paragraph to say what it does, then explain and defend why it is a good change in later paragraphs" convention.

This change is a two-edged sword. While the earlier behaviour would make such commit log messages more friendly to readers who expect to get the birds-eye view with oneline summary formats, users who primarily use git as a way to interact with foreign SCM systems would not care much about the convenience of oneline git log tools, but care more about preserving their own convention. This changes their commits less useful to readers who read them with git tools while keeping them more consistent with the foreign SCM systems they interact with.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-mailinfo.c          |   29 +++++++++++++++++++++----
 t/t3405-rebase-malformed.sh |   48 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 5 deletions(-)
 create mode 100755 t/t3405-rebase-malformed.sh

Junio C Hamano wrote:

I think that git should not enforce this policy. Think import and exchange with foreign SCMs which do not follow this, argueably very reasonable, one-line summary policy.

lördag 28 juli 2007 skrev Junio C Hamano:

git-svn should not enforce git conventions when managing commits intended for svn. Those commits should obviously follow the conventions for the target (svn) repo.

-- robin

On Jul 28, 2007, at 3:10 PM, Junio C Hamano wrote:

Hi people, thanks for the quick and complete replies. As a user, I do not expect git-rebase to change my commit message. I was aware of this convention in Git, but it looks like it's more than just a convention. This trap should either be fixed (your patch works for me) or it should be clearly stated in the documentation that commit messages must really be formatted according to the convention.

Personally I expected Git to keep the 1st line of my commit message as the "short version" (which is what git log and the like do ATM) and although I don't leave a blank line between the 1st line and the rest of the message, the 1st line is always the sentence that can be used as a short version of the commit message.

Cheers,