git rebase -i / git-gui bug

git rebase -i / git-gui bug

[Bernt Hansen]

Thanks for a great tool I use everyday!

I'm using the tip of master for my regular day job with git-gui to write
the commit messages.  I've run into a little glitch with git rebase -i
and git-gui.

$ git --version
git version 1.5.4.rc0.73.gce85b

I haven't been able to automate the process of reproducing this but the
steps are fairly simple.

To show the problem you can create a test repository using the following
script:

--- t1.sh ---
#!/bin/sh
cd /tmp
mkdir t1
cd t1
git init

for F in f1 f2 f3 f4 f5 f6 f7 f8 f9 f10; do
  echo $F >$F
  git add $F
  echo -n -e "Commit for $F\n\nThis is line one\nThis is line two" >/tmp/commitmsg.txt 
  git commit -F /tmp/commitmsg.txt
done
-------------

Now cd /tmp/t1 and do the following:

$ git rebase -i HEAD~9

Change all lines with 'pick' to 'edit'

For each of the 10 commits use git-gui to select 'Amend Last Commit' and
just hit the [Commit] button (you can change the text if you want but
it's not necessary to show the problem)

$ git rebase --continue
after each commit and repeat until the rebase is complete.

Now if you try to squash these commits the edit buffer commit text is a
little mangled.

$ git rebase -i HEAD~9

Change all but the first pick line to 'squash' and I get the following
text in the commit message edit window:

----[ /tmp/t1/.git/COMMIT_EDITMSG ]----
# This is a combination of 3 commits.
# The first commit's message is:

Commit for f2

This is line one
This is line two
# This is the 2nd commit message:

Commit for f3

This is line one
This is line two# This is the 3rd commit message:

Commit for f4

This is line one
This is line two# This is the 3rd commit message:

Commit for f5

This is line one
This is line two# This is the 3rd commit message:

Commit for f6

This is line one
This is line two# This is the 3rd commit message:

Commit for f7

This is line one
This is line two# This is the 3rd commit message:

Commit for f8

This is line one
This is line two# This is the 3rd commit message:

Commit for f9

This is line one
This is line two# This is the 3rd commit message:

Commit for f10

This is line one
This is line two

# Please enter the commit message for your changes.
# (Comment lines starting with '#' will not be included)
# Not currently on any branch.
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	new file:   f10
#	new file:   f2
#	new file:   f3
#	new file:   f4
#	new file:   f5
#	new file:   f6
#	new file:   f7
#	new file:   f8
#	new file:   f9
#
---------------------------------------

Notice after the 3rd commit the '# This is the 3rd commit message:' is
appended to the last line of the previous commit message and the counter
seems to be stuck on 3.

---

When I run into this during work I fix up the commit text by adding
newlines before the '# This is the 3rd commit message' and it works
fine.

This might be the lack of a newline after the last line in the commit
edit message when git-gui creates the commit -- maybe.

If I use git --amend instead of git-gui to update the commits on the
above test repository it works correctly.

I'm posting this because someone else can probably fix this faster than
me (I've never looked at the git source code).  I'll post a patch when I
figure it out if nobody else beats me to it.

Regards,
Bernt

Re: git rebase -i / git-gui bug

[Bernt Hansen]

Bernt Hansen <bernt@alumni.uwaterloo.ca> writes:

> Now cd /tmp/t1 and do the following:
>
> $ git rebase -i HEAD~9
>
> Change all lines with 'pick' to 'edit'
>
> For each of the 10 commits use git-gui to select 'Amend Last Commit' and
> just hit the [Commit] button (you can change the text if you want but
> it's not necessary to show the problem)
>
> $ git rebase --continue
> after each commit and repeat until the rebase is complete.
>

I can't do this at all with

$ git --version
git version 1.5.4.rc1

If I

$ git rebase -i HEAD~9

and use git-gui to edit the commit then git-rebase --continue fails

$ git rebase --continue
Could not commit staged changes.

-Bernt

[PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Shawn O. Pearce]

During git-rebase --interactive's --continue implementation we used
to silently restart the rebase if the user had made the commit
for us.  This is common if the user stops to edit a commit and
does so by amending it.  My recent change to watch git-commit's
exit status broke this behavior.

Thanks to Bernt Hansen for catching it in 1.5.4-rc1.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 47581ce..39f32b1 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -372,8 +372,9 @@ do
 			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
 		} &&
 		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-		git commit --no-verify -F "$DOTEST"/message -e ||
+		if ! git commit --no-verify -F "$DOTEST"/message -e
 			die "Could not commit staged changes."
+		fi
 
 		require_clean_work_tree
 		do_rest
-- 
1.5.4.rc1.1090.gab2276

Re: [PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> During git-rebase --interactive's --continue implementation we used
> to silently restart the rebase if the user had made the commit
> for us.  This is common if the user stops to edit a commit and
> does so by amending it.  My recent change to watch git-commit's
> exit status broke this behavior.
>
> Thanks to Bernt Hansen for catching it in 1.5.4-rc1.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  git-rebase--interactive.sh |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 47581ce..39f32b1 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -372,8 +372,9 @@ do
>  			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
>  		} &&
>  		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> -		git commit --no-verify -F "$DOTEST"/message -e ||
> +		if ! git commit --no-verify -F "$DOTEST"/message -e
>  			die "Could not commit staged changes."
> +		fi

This looks like a syntax error to me.

Re: [PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 47581ce..39f32b1 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -372,8 +372,9 @@ do
> >  			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
> >  		} &&
> >  		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> > -		git commit --no-verify -F "$DOTEST"/message -e ||
> > +		if ! git commit --no-verify -F "$DOTEST"/message -e
> >  			die "Could not commit staged changes."
> > +		fi
> 
> This looks like a syntax error to me.

Whoops.  This looks like a syntax error to me too.

Its late.  I totally missed a "then".  Would you mind doing an amend?

-- 
Shawn.

Re: [PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Matthieu Moy]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> -		git commit --no-verify -F "$DOTEST"/message -e ||
                                                                  ^
Wouldn't it be sufficient to add a backslash here ----------------' ?

> +		if ! git commit --no-verify -F "$DOTEST"/message -e
>  			die "Could not commit staged changes."
> +		fi
>  
>  		require_clean_work_tree
>  		do_rest

-- 
Matthieu

Re: [PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Junio C Hamano]

Will do, but the code looks quite bad (not entirely your fault).

Line by line comment to show my puzzlement.

 		# commit if necessary

Ok, the user has prepared the index for us, and we are going to do some
tests and conditionally create commit.

 		git rev-parse --verify HEAD > /dev/null &&

Do we have HEAD commit?  Why check this --- we do not want to rebase
from the beginning of time?  No, that's not it.  If this fails, there is
something seriously wrong.  This is not about "will we make a commit?"
check at all.  This is a basic sanity check and if it fails we must
abort, not just skip.

 		git update-index --refresh &&
 		git diff-files --quiet &&

Is the work tree clean with respect to the index?  Why check this --- we
want to skip the commit if work tree is dirty?  Or is this trying to
enforce the invariant that during the rebase the work tree and index and
HEAD should all match?  If the latter, failure from this again is a
reason to abort.

 		! git diff-index --cached --quiet HEAD -- &&

Do we have something to commit?  This needs to be checked so that we can
skip a commit that results in emptyness, so using this as a check to see
if we should commit makes sense.

 		. "$DOTEST"/author-script && {
 			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
 		} &&

Find GIT_AUTHOR_* variables and if we are amending rewind the HEAD.  The
failure from this is a grave problem and reason to abort, isn't it?

 		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
		git commit --no-verify -F "$DOTEST"/message -e

Then we go on to create commit.  As you said, failure from this is a
grave error.

If my commentary above is right, how many bugs did we find in these 10
lines?

Grumpy I am...

---
 git-rebase--interactive.sh |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 090c3e5..7aa4278 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -363,17 +363,26 @@ do
 
 		test -d "$DOTEST" || die "No interactive rebase running"
 
-		# commit if necessary
-		git rev-parse --verify HEAD > /dev/null &&
-		git update-index --refresh &&
-		git diff-files --quiet &&
-		! git diff-index --cached --quiet HEAD -- &&
-		. "$DOTEST"/author-script && {
-			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
-		} &&
-		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-		if ! git commit --no-verify -F "$DOTEST"/message -e
+		# Sanity check
+		git rev-parse --verify HEAD >/dev/null ||
+			die "Cannot read HEAD"
+		git update-index --refresh && git diff-files --quiet ||
+			die "Working tree is dirty"
+
+		# do we have anything to commit?
+		if git diff-index --cached --quiet HEAD --
 		then
+			: Nothing to commit -- skip this
+		else
+			. "$DOTEST"/author-script ||
+				die "Cannot find the author identity"
+			if test -f "$DOTEST"/amend
+			then
+				git reset --soft HEAD^ ||
+				die "Cannot rewind the HEAD"
+			fi
+			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
+			git commit --no-verify -F "$DOTEST"/message -e ||
 			die "Could not commit staged changes."
 		fi
 

[PATCH] Force new line at end of commit message

[Bernt Hansen]

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 <bernt@alumni.uwaterloo.ca>
---

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

Re: [PATCH] Force new line at end of commit message

[Johannes Schindelin]

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

Re: [PATCH] Force new line at end of commit message

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 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...)

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.

Re: [PATCH] Force new line at end of commit message

[Shawn O. Pearce]

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

Bernt Hansen <bernt@alumni.uwaterloo.ca> 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.

Re: [PATCH] Force new line at end of commit message

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> 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.

Re: [PATCH] Force new line at end of commit message

[Bernt Hansen]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> 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 :)

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

Re: [PATCH] Force new line at end of commit message

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

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

I would not go that far, even though I would agree that the
consumers of existing commits should be lenient and the creators
of new commits should be strict.

Now, "strict" and "lenient" are both relative to some yardstick,
but relative to what?  I would say that the UI layer of "git the
SCM" is about helping humans create commit messages for human
consumption, even though the low-level commit objects are
equipped to record any binary blob (including NUL byte).

As UI layer programs, I think "git commit" and "git rebase -i"
can and should be stricter than allowing "arbitrary binary
blobs".  Namely, they should make sure what they produce are
good text messages (and a good text message ends with a LF ---
prepare a file with an incomplete line, run "cat file" from
interactive shell on it, and see your prompt tucked at the end
before arguing otherwise).

So how about doing something like this?

---
 git-rebase--interactive.sh |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 090c3e5..d0d83c3 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -215,15 +215,17 @@ make_squash_message () {
 		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
 			< "$SQUASH_MSG" | tail -n 1)+1))
 		echo "# This is a combination of $COUNT commits."
-		sed -n "2,\$p" < "$SQUASH_MSG"
+		sed -e 1d -e '2,/^./{
+			/^$/d
+		}' <"$SQUASH_MSG"
 	else
 		COUNT=2
 		echo "# This is a combination of two commits."
 		echo "# The first commit's message is:"
 		echo
 		git cat-file commit HEAD | sed -e '1,/^$/d'
-		echo
 	fi
+	echo
 	echo "# This is the $(nth_string $COUNT) commit message:"
 	echo
 	git cat-file commit $1 | sed -e '1,/^$/d'

Re: [PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Junio C Hamano]

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

> Will do, but the code looks quite bad (not entirely your fault).
>
> Line by line comment to show my puzzlement.
>
>  		# commit if necessary
>
> Ok, the user has prepared the index for us, and we are going to do some
> tests and conditionally create commit.
>
>  		git rev-parse --verify HEAD > /dev/null &&
>
> Do we have HEAD commit?  Why check this --- we do not want to rebase
> from the beginning of time?  No, that's not it.  If this fails, there is
> something seriously wrong.  This is not about "will we make a commit?"
> check at all.  This is a basic sanity check and if it fails we must
> abort, not just skip.
>
>  		git update-index --refresh &&
>  		git diff-files --quiet &&
>
> Is the work tree clean with respect to the index?  Why check this --- we
> want to skip the commit if work tree is dirty?  Or is this trying to
> enforce the invariant that during the rebase the work tree and index and
> HEAD should all match?  If the latter, failure from this again is a
> reason to abort.
>
>  		! git diff-index --cached --quiet HEAD -- &&
>
> Do we have something to commit?  This needs to be checked so that we can
> skip a commit that results in emptyness, so using this as a check to see
> if we should commit makes sense.
>
>  		. "$DOTEST"/author-script && {
>  			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
>  		} &&
>
> Find GIT_AUTHOR_* variables and if we are amending rewind the HEAD.  The
> failure from this is a grave problem and reason to abort, isn't it?
>
>  		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> 		git commit --no-verify -F "$DOTEST"/message -e
>
> Then we go on to create commit.  As you said, failure from this is a
> grave error.

Any response to this or problems in the clean-up patch?

> ---
>  git-rebase--interactive.sh |   29 +++++++++++++++++++----------
>  1 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 090c3e5..7aa4278 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -363,17 +363,26 @@ do
>  
>  		test -d "$DOTEST" || die "No interactive rebase running"
>  
> -		# commit if necessary
> -		git rev-parse --verify HEAD > /dev/null &&
> -		git update-index --refresh &&
> -		git diff-files --quiet &&
> -		! git diff-index --cached --quiet HEAD -- &&
> -		. "$DOTEST"/author-script && {
> -			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
> -		} &&
> -		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> -		if ! git commit --no-verify -F "$DOTEST"/message -e
> +		# Sanity check
> +		git rev-parse --verify HEAD >/dev/null ||
> +			die "Cannot read HEAD"
> +		git update-index --refresh && git diff-files --quiet ||
> +			die "Working tree is dirty"
> +
> +		# do we have anything to commit?
> +		if git diff-index --cached --quiet HEAD --
>  		then
> +			: Nothing to commit -- skip this
> +		else
> +			. "$DOTEST"/author-script ||
> +				die "Cannot find the author identity"
> +			if test -f "$DOTEST"/amend
> +			then
> +				git reset --soft HEAD^ ||
> +				die "Cannot rewind the HEAD"
> +			fi
> +			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> +			git commit --no-verify -F "$DOTEST"/message -e ||
>  			die "Could not commit staged changes."
>  		fi
>  

Re: [PATCH] Force new line at end of commit message

[Shawn O. Pearce]

Bernt Hansen <bernt@alumni.uwaterloo.ca> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> >
> > 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.

[PATCH] git-gui: Make commit log messages end with a newline

[Bernt Hansen]

Concatenating commit log messages from multiple commits works better
when all of the commits end with a clean line break.

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

Signed-off-by: Bernt Hansen <bernt@alumni.uwaterloo.ca>
---
 lib/commit.tcl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/commit.tcl b/lib/commit.tcl
index b2d2d53..1c0586c 100644
--- a/lib/commit.tcl
+++ b/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.21.g0e545

Re: [PATCH] Reallow git-rebase --interactive --continue if commit is unnecessary

[Johannes Schindelin]

Hi,

On Wed, 26 Dec 2007, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >  		# commit if necessary
> >
> > Ok, the user has prepared the index for us, and we are going to do some
> > tests and conditionally create commit.
> >
> >  		git rev-parse --verify HEAD > /dev/null &&
> >
> > Do we have HEAD commit?  Why check this --- we do not want to rebase
> > from the beginning of time?  No, that's not it.  If this fails, there is
> > something seriously wrong.  This is not about "will we make a commit?"
> > check at all.  This is a basic sanity check and if it fails we must
> > abort, not just skip.

Right.  My intention was to have a "|| die" at the end of the && cascade.

> >
> >  		git update-index --refresh &&
> >  		git diff-files --quiet &&
> >
> > Is the work tree clean with respect to the index?  Why check this --- we
> > want to skip the commit if work tree is dirty?  Or is this trying to
> > enforce the invariant that during the rebase the work tree and index and
> > HEAD should all match?  If the latter, failure from this again is a
> > reason to abort.

Exactly.  I wanted to make sure that the working directory is not 
different from what is in the index.

> >  		! git diff-index --cached --quiet HEAD -- &&
> >
> > Do we have something to commit?  This needs to be checked so that we can
> > skip a commit that results in emptyness, so using this as a check to see
> > if we should commit makes sense.
> >
> >  		. "$DOTEST"/author-script && {
> >  			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
> >  		} &&
> >
> > Find GIT_AUTHOR_* variables and if we are amending rewind the HEAD.  The
> > failure from this is a grave problem and reason to abort, isn't it?
> >
> >  		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> > 		git commit --no-verify -F "$DOTEST"/message -e
> >
> > Then we go on to create commit.  As you said, failure from this is a
> > grave error.
> 
> Any response to this or problems in the clean-up patch?
> 
> > ---
> >  git-rebase--interactive.sh |   29 +++++++++++++++++++----------
> >  1 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 090c3e5..7aa4278 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -363,17 +363,26 @@ do
> >  
> >  		test -d "$DOTEST" || die "No interactive rebase running"
> >  
> > -		# commit if necessary
> > -		git rev-parse --verify HEAD > /dev/null &&
> > -		git update-index --refresh &&
> > -		git diff-files --quiet &&
> > -		! git diff-index --cached --quiet HEAD -- &&
> > -		. "$DOTEST"/author-script && {
> > -			test ! -f "$DOTEST"/amend || git reset --soft HEAD^
> > -		} &&
> > -		export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> > -		if ! git commit --no-verify -F "$DOTEST"/message -e
> > +		# Sanity check
> > +		git rev-parse --verify HEAD >/dev/null ||
> > +			die "Cannot read HEAD"
> > +		git update-index --refresh && git diff-files --quiet ||
> > +			die "Working tree is dirty"
> > +
> > +		# do we have anything to commit?
> > +		if git diff-index --cached --quiet HEAD --
> >  		then
> > +			: Nothing to commit -- skip this
> > +		else
> > +			. "$DOTEST"/author-script ||
> > +				die "Cannot find the author identity"
> > +			if test -f "$DOTEST"/amend
> > +			then
> > +				git reset --soft HEAD^ ||
> > +				die "Cannot rewind the HEAD"
> > +			fi
> > +			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> > +			git commit --no-verify -F "$DOTEST"/message -e ||
> >  			die "Could not commit staged changes."
> >  		fi

Looks good to me!  And I'm sorry for taking so long to respond.

Ciao,
Dscho

Re: [PATCH] Force new line at end of commit message

[Johannes Schindelin]

Hi,

On Wed, 26 Dec 2007, Junio C Hamano wrote:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 090c3e5..d0d83c3 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -215,15 +215,17 @@ make_squash_message () {
>  		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
>  			< "$SQUASH_MSG" | tail -n 1)+1))
>  		echo "# This is a combination of $COUNT commits."
> -		sed -n "2,\$p" < "$SQUASH_MSG"
> +		sed -e 1d -e '2,/^./{
> +			/^$/d
> +		}' <"$SQUASH_MSG"

If I read this correctly (haven't tested), then _all_ empty lines are 
removed from the SQUASH_MSG, right?  This is not what I want.

I had something like this in mind, rather:

		-e '\$s/\n*$/\n/'

This is completely untested, but should show the idea.  However, the same 
must go into this clause:

>  	else
>  		COUNT=2
>  		echo "# This is a combination of two commits."
>  		echo "# The first commit's message is:"
>  		echo
>  		git cat-file commit HEAD | sed -e '1,/^$/d'

Unfortunately, I am unable to provide a proper patch with proper testing 
right now, since my family threatens me with physical violence, should I 
not leave the keyboard right n.. OUCH!

Re: [PATCH] Force new line at end of commit message

[Junio C Hamano]

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

>>  		echo "# This is a combination of $COUNT commits."
>> -		sed -n "2,\$p" < "$SQUASH_MSG"
>> +		sed -e 1d -e '2,/^./{
>> +			/^$/d
>> +		}' <"$SQUASH_MSG"
>
> If I read this correctly (haven't tested), then _all_ empty lines are 
> removed from the SQUASH_MSG, right?  This is not what I want.

That is no what I wanted either.

"1d" removes the "# This is a combination of <$n> commits." from
the previous round, "2,/^./{ ... }" says apply what's in {} to
lines from second line to the first non-empty line, and what's
in {} is to remove empty ones.

> Unfortunately, I am unable to provide a proper patch with proper testing 
> right now,...

That's Ok.  Seeing what the patch already in front of you does
would be less time consuming.

Re: [PATCH] Force new line at end of commit message

[Johannes Schindelin]

Hi,

On Sat, 29 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >>  		echo "# This is a combination of $COUNT commits."
> >> -		sed -n "2,\$p" < "$SQUASH_MSG"
> >> +		sed -e 1d -e '2,/^./{
> >> +			/^$/d
> >> +		}' <"$SQUASH_MSG"
> >
> > If I read this correctly (haven't tested), then _all_ empty lines are 
> > removed from the SQUASH_MSG, right?  This is not what I want.
> 
> That is no what I wanted either.
> 
> "1d" removes the "# This is a combination of <$n> commits." from
> the previous round, "2,/^./{ ... }" says apply what's in {} to
> lines from second line to the first non-empty line, and what's
> in {} is to remove empty ones.

Okay, so it removes only the empty lines between the first line and the 
next non-empty line.

But if I understood the OP correctly, the problem was a missing newline at 
the end of the commit message, no?

Thanks,
Dscho

Re: [PATCH] Force new line at end of commit message

[Junio C Hamano]

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

> But if I understood the OP correctly, the problem was a missing newline at 
> the end of the commit message, no?

That's why the "echo" was moved out of the conditional, to make
sure "# This is the $(nth)" begins on a fresh line.

Re: [PATCH] Force new line at end of commit message

[Johannes Schindelin]

Hi,

On Sun, 30 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > But if I understood the OP correctly, the problem was a missing 
> > newline at the end of the commit message, no?
> 
> That's why the "echo" was moved out of the conditional, to make sure "# 
> This is the $(nth)" begins on a fresh line.

Not that I care too deeply, but does that not add a newline regardless 
whether it is needed or not?

Thanks,
Dscho

Re: [PATCH] Force new line at end of commit message

[Junio C Hamano]

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

> Not that I care too deeply, but does that not add a newline regardless 
> whether it is needed or not?

Heh, I can see that you do not care---the original did not even
add a newline when necessary (and that is why we have this
thread).  Instead you were adding a newline regardless to the
end of the first commit, but not doing so for the other ones.

The patch just moves that unconditional "echo"; instead of
adding one to the end of the first commit (and only the first
one), it adds before the new commit's title message.

Re: [PATCH] Force new line at end of commit message

[しらいしななこ]

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Not that I care too deeply, but does that not add a newline regardless 
>> whether it is needed or not?
>
> Heh, I can see that you do not care---the original did not even
> add a newline when necessary (and that is why we have this
> thread).  Instead you were adding a newline regardless to the
> end of the first commit, but not doing so for the other ones.

Aren't you being too harsh on Johannes these days?

Everybody knows that you are capable of rewriting that part in Perl or Python yourself to fix the issue.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

----------------------------------------------------------------------
Get a free email address with REAL anti-spam protection.
http://www.bluebottle.com/tag/1

Re: [PATCH] Force new line at end of commit message

[Junio C Hamano]

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

> ...  Instead you were adding a newline regardless to the
> end of the first commit, but not doing so for the other ones.

To illustrate, this is what I get when trying to squash four
commits:

    # This is a combination of 4 commits.
    # The first commit's message is:

    Documentation/git-submodule.txt: typofix

    Signed-off-by: Junio C Hamano <gitster@pobox.com>

    # This is the 2nd commit message:

    git-sh-setup: document git_editor() and get_author_ident_from_commit()

    These 2 functions were missing from the manpage.

    Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
    # This is the 3rd commit message:

    "git pull --tags": error out with a better message.

    When "git pull --tags" is run without any other arguments, the
    ...

Notice that there is a gap before "# This is the 2nd commit" but
there isn't any gap before "# This is the 3rd commit"?

The patch under discussion happens to fix this inconsistency as
a side effect.

Re: [PATCH] Force new line at end of commit message

[Junio C Hamano]

しらいしななこ  <nanako3@bluebottle.com> writes:

>> Heh, I can see that you do not care---the original did not even
>> add a newline when necessary (and that is why we have this
>> thread).  Instead you were adding a newline regardless to the
>> end of the first commit, but not doing so for the other ones.
>
> Aren't you being too harsh on Johannes these days?

Not on purpose, but perhaps I might have been.

> Everybody knows that you are capable of rewriting that part in Perl or Python yourself to fix the issue.

I actually have been trying to avoid Perl (let alone Python nor
Ruby) as "rebase -i" is primarily Johannes's bailiwick, and I
had an impression that he avoided them for Windows portability.

Unfortunately, sed does not handle incomplete lines well, at
least portably.  POSIX says very little about it, except that
its input shall be "text files" (i.e. no NUL is allowed, each
line separated with <newline> and with less than {LINE_MAX}
bytes in length), and its default operation shall read each line
less its terminating <newline> and after manipulation spit it
out and immediately follow it with a <newline>.  But a popular
implementation (e.g. GNU) actually does not follow the output
with a <newline> if the input was incomplete line [*1*]

[Footnote]

*1* Otherwise, this would have been a way to add a
missing newline to a file that could end with an incomplete
line:

    $ sed -e '' <$file_that_may_end_with_an_incomplete_line

Re: [PATCH] Force new line at end of commit message

[Johannes Schindelin]

Hi,

[Bernt, your mail filter is less than intelligent and rejects my mails.]

On Sun, 30 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Not that I care too deeply, but does that not add a newline regardless 
> > whether it is needed or not?
> 
> Heh, I can see that you do not care---the original did not even
> add a newline when necessary (and that is why we have this
> thread).

Umm.  It was on purpose, since I found the empty lines between the commit 
messages and the comment more pleasing than no empty space.

> The patch just moves that unconditional "echo"; instead of adding one to 
> the end of the first commit (and only the first one), it adds before the 
> new commit's title message.

Well, ACK from me, then.

Ciao,
Dscho