git rebase -i / git-gui bug

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

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

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

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

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

This looks like a syntax error to me.

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

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

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

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

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

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

Hi,

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

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

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

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

Ciao, Dscho

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

                                                                  ^
Wouldn't it be sufficient to add a backslash here ----------------' ?

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

Hi,

On Mon, 24 Dec 2007, Bernt Hansen 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...)

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

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.
 

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

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" <spearce@spearce.org> writes:

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

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

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.
 

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

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.

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

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

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

Hi,

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

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:

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!

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

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.

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

Hi,

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

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

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

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

Hi,

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

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

Thanks, Dscho

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

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.

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

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.

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

Not on purpose, but perhaps I might have been.

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

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

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.

Hi,

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

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

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

Well, ACK from me, then.

Ciao, Dscho

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

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

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.