[PATCH] Force new line at end of commit message

git rebase --interactive formats the combined commit log message
incorrectly when squashing 3 or more commits which have no newline on
the last line of the commit message.

Signed-off-by: Bernt Hansen <redacted>
---

This may well be the wrong fix for this problem but my attempts to make
git-rebase--interactive.sh append a newline breaks too many tests in the
test suite.

I tried something like this in git-rebase--interactive.sh:

-               git cat-file commit HEAD | sed -e '1,/^$/d'
+               git cat-file commit HEAD | sed -e '1,/^$/d' -e '$a\'

Sorry I don't have an automated test for git-gui.  Are there any?

 git-gui/lib/commit.tcl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index b2d2d53..1c0586c 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl

@@ -303,7 +303,7 @@ A rescan will be automatically started now.
 		puts stderr [mc "warning: Tcl does not support encoding '%s'." $enc]
 		fconfigure $msg_wt -encoding utf-8
 	}
-	puts -nonewline $msg_wt $msg
+	puts $msg_wt $msg
 	close $msg_wt
 
 	# -- Create the commit.

-- 
1.5.4.rc1.22.g88b9

Hi,

On Mon, 24 Dec 2007, Bernt Hansen wrote:

git rebase --interactive formats the combined commit log message 
incorrectly when squashing 3 or more commits which have no newline on 
the last line of the commit message.

This is a patch for git-gui, so why not make that clear in the subject?  
(And I have a hunch that Shawn would have liked the patch relative to 
git-gui.git, not git.git...)

Further, there are other tools than rebase -i that like commit messages 
better when terminated by a newline, and _that_ is what I would like to 
read in the commit message for this patch.

If nobody is quicker, I'll try to fix the problem on the rebase -i side in 
a few days.

Thanks,
Dscho

Johannes Schindelin [off-list ref] wrote:

On Mon, 24 Dec 2007, Bernt Hansen wrote:

quoted

git rebase --interactive formats the combined commit log message 
incorrectly when squashing 3 or more commits which have no newline on 
the last line of the commit message.

This is a patch for git-gui, so why not make that clear in the subject?  
(And I have a hunch that Shawn would have liked the patch relative to 
git-gui.git, not git.git...)

Indeed.

Most git-gui changes have a subject that starts with "git-gui:" so
its clear in both the email and in the commit log that the change is
a git-gui change.  Remember, git-gui's logs show up in the core Git
logs (as its merged with -s subtree) so having that git-gui: prefix
does help people to localize the change within the overall suite.

git-am -3 does a reasonable job at correcting patches that are like
this one is (that aren't relative to git-gui.git) so that's less
of an issue for me.  And what git-am -3 cannot correct git-apply
-p2 usually does.  If that can't fix the patch then I'll usually
throw it back as its then most likely a true conflict.
 

Further, there are other tools than rebase -i that like commit messages 
better when terminated by a newline, and _that_ is what I would like to 
read in the commit message for this patch.

Hmmph.  For that reason alone I'm tempted to *not* apply Bernt's
patch.

There is nothing that requires that a commit object end with an LF.
So tools that make this assumption (that there is a trailing LF)
while processing the body of a commit message are quite simply
broken.

Its easy in fast-import to generate commits without a trailing LF.
Or in many text editors its possible to save a file with no trailing
LF on the last line.  My favorite VI clone does that; if the file
doesn't end with an LF when it opens its *damned* hard to get a
trailing LF onto that last line.  And yes, that's the editor I use
for commit messages when I'm not using git-gui.

IMHO git-gui is producing valid commit messages, and always does
so with no trailing LF, and any tool that is assuming a trailing
LF is always present is broken.

Keeping git-gui behavior like this actually highlights the other
tools that are broken (here Bernt found git-rebase--interactive).


I'd like to hear Junio's or Linus' two cents on the matter, but
if we really want to say that all commits must end with an LF then
maybe git-commit-tree, git-hash-object and git-fast-import should be
performing that sort of validation before creating such an object in
the ODB.  Which is probably a change that shouldn't be made before
1.6.0 as its somewhat likely to break people's existing scripts.

-- 
Shawn.

[... comments for patch in reply to Dscho's reply...]

Bernt Hansen [off-list ref] wrote:

Sorry I don't have an automated test for git-gui.  Are there any?

No.  I didn't really build git-gui very well for that sort of thing.
Part of my long-term plan for git-gui is to do refactoring on it
so that we can create automated tests for the lower level parts
(the logic behind the GUI).  Then we can actually do some automated
testing.

Wow.  I just realized git-gui is almost 14 months old.  Its probably
going to be another year before the above said refactoring is
completely finished, but its something that needs to be done if
git-gui is going to survive its terrible twos.

-- 
Shawn.

"Shawn O. Pearce" [off-list ref] writes:

I'd like to hear Junio's or Linus' two cents on the matter, but
if we really want to say that all commits must end with an LF then
maybe git-commit-tree, git-hash-object and git-fast-import should be
performing that sort of validation before creating such an object in
the ODB.

I've so far tried to keep the lowest-level plumbing commit-tree
(and even lower hash-object) without such an artificial limit.
At the lowest level, commit objects should be able to hold any
byte sequence (this includes NUL bytes) as the user wishes.
People who want to use git to implement/experiment a data
structure that may not have anything to do with the usual SCM
should be able to do so using such low-level.

It is a different story about what conventions should Porcelains
enforce.  For example, I'd be perfectly happy if git-commit (at
least under its default mode of operation) does not allow NULs
nor incomplete lines in the message, and if git-format-patch and
git-am do not to pass something you cannot e-mail sanely (but
that is only true once we rewrite rebase not to rely on the
pipeline between them).  Porcelain level should really make it
easy and safe for the users to work with git as an SCM.

"Shawn O. Pearce" [off-list ref] writes:

Johannes Schindelin [off-list ref] wrote:

quoted

This is a patch for git-gui, so why not make that clear in the subject?  
(And I have a hunch that Shawn would have liked the patch relative to 
git-gui.git, not git.git...)

Indeed.

its clear in both the email and in the commit log that the change is
a git-gui change.  Remember, git-gui's logs show up in the core Git
logs (as its merged with -s subtree) so having that git-gui: prefix
does help people to localize the change within the overall suite.

Thanks for the feedback on the patch.

This is my first attempt at creating a patch for git (even if it is
mostly trivial in this case) and I wasn't aware of the git-gui.gitk repo
and conventions regarding the commit message.  I just tried to follow
what was in Documentation/SubmittingPatches.  I'll try to do better next
time :)

quoted

Further, there are other tools than rebase -i that like commit messages 
better when terminated by a newline, and _that_ is what I would like to 
read in the commit message for this patch.

Hmmph.  For that reason alone I'm tempted to *not* apply Bernt's
patch.

There is nothing that requires that a commit object end with an LF.
So tools that make this assumption (that there is a trailing LF)
while processing the body of a commit message are quite simply
broken.

Forcing a LF on the end of the commit message feels wrong to me too.

This band-aid solution fixes the issue I'm dealing with for
git-rebase -i when squashing 3 or more commits created by git-gui.

I agree with Sean and think the more correct fix would be to make
git rebase -i and any other tools we encounter with similar problems
handle the case where there is no newline at the end of the commit
message.

The patch as it stands should probably not be applied.

-Bernt

Bernt Hansen [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

its clear in both the email and in the commit log that the change is
a git-gui change.  Remember, git-gui's logs show up in the core Git
logs (as its merged with -s subtree) so having that git-gui: prefix
does help people to localize the change within the overall suite.

This is my first attempt at creating a patch for git (even if it is
mostly trivial in this case) and I wasn't aware of the git-gui.gitk repo
and conventions regarding the commit message.  I just tried to follow
what was in Documentation/SubmittingPatches.  I'll try to do better next
time :)

Its a good first attempt.  I also just sent a patch to Junio to try
and make this "special case" of directing git-gui changes to me more
clear for new folk.
 

Forcing a LF on the end of the commit message feels wrong to me too.

I think Junio just convinced me otherwise.

We probably should change git-gui to always end the last line of
the message with an LF.  To be honest I'm not really sure why it
doesn't do that now.  ;-)
 

The patch as it stands should probably not be applied.

But I think that is now only because the commit message could be
clarified to state that its for git-gui (e.g. start with "git-gui:")
and probably shouldn't be so specific to rebase -i's breakage but
instead talk about how its good to be strict in what you create,
and lenient in what you accept, and since we're creating here,
we should always try to Do The Right Thing(tm).

If you respin the patch with a more descriptive message I'll put
it into 0.9.1.

-- 
Shawn.