git-rebase -i prunes commits with empty commit-message

I'm in the process of converting an SVN repo to Git, and in the
process I found one quite disturbing feature of
git-rebase--interactive.sh: It discards commits with empty commit
messages!

Here's a recepie for reproducing the issue:
--->8---
git init
git commit -m "dummy" --allow-empty
git commit -m "dummy" --allow-empty
git commit -m "dummy" --allow-empty
git filter-branch -f --msg-filter 'sed -e "s/dummy//"'
git rebase -i HEAD~2
--->8---

The editor window will show "noop", and exiting the editor goes ahead
and deletes all but the initial commit.

This gets even weirder if it's a mixture of empty and non-empty
commits; the commit-identifiers gets appended, together with an
angle-bracket ('>'), to the previous line.

I'm guessing that this is unintended behavior. This was observed on git 1.7.0.1.

-- 
Erik "kusma" Faye-Lund

Erik Faye-Lund wrote:

I'm in the process of converting an SVN repo to Git, and in the
process I found one quite disturbing feature of
git-rebase--interactive.sh: It discards commits with empty commit
messages!

Here's a recepie for reproducing the issue:
--->8---
git init
git commit -m "dummy" --allow-empty
git commit -m "dummy" --allow-empty
git commit -m "dummy" --allow-empty
git filter-branch -f --msg-filter 'sed -e "s/dummy//"'
git rebase -i HEAD~2
--->8---

Does git really claim to handle commits with empty commit messages?
That you have to use git-filter-branch to create the test case suggests
that the answer is "no", but I don't know.  (git-commit, for example,
refuses to create a commit with an empty message.)

If indeed git requires commit messages to be non-empty, then the fault
here seemingly lies with git-filter-branch for allowing commit messages
to be completely deleted.

Michael

On Wed, Mar 10, 2010 at 2:13 PM, Michael Haggerty [off-list ref] wrote:

Erik Faye-Lund wrote:

quoted

I'm in the process of converting an SVN repo to Git, and in the
process I found one quite disturbing feature of
git-rebase--interactive.sh: It discards commits with empty commit
messages!

Here's a recepie for reproducing the issue:
--->8---
git init
git commit -m "dummy" --allow-empty
git commit -m "dummy" --allow-empty
git commit -m "dummy" --allow-empty
git filter-branch -f --msg-filter 'sed -e "s/dummy//"'
git rebase -i HEAD~2
--->8---

Does git really claim to handle commits with empty commit messages?
That you have to use git-filter-branch to create the test case suggests
that the answer is "no", but I don't know.  (git-commit, for example,
refuses to create a commit with an empty message.)

If git didn't, I'd expect it to be impossible to create them. This was
just a minimal example on how to reproduce it, and it's a silly
use-case. I think my original way of getting into the state was a tad
more legitimate:
1) I had an SVN-repo with empty commit-messages
2) I imported that SVN-repo into git through git-svn, and git-svn
appended it's meta-data to the otherwise empty commit-message.
3) I used git-filter-branch to remove the git-svn metadata (as the
git-filter-branch man-page suggest)

I don't know what happens when git-svn encounter empty commit-messages
with the --no-metadata option enabled. In my case, the decision to
fully migrate the repo to Git (instead of just using git-svn as a
nicer svn-frontend) came long after 1) and 2) were done.

If indeed git requires commit messages to be non-empty, then the fault
here seemingly lies with git-filter-branch for allowing commit messages
to be completely deleted.

If there is some decided-upon restriction that commit-messages cannot
be empty, then I agree with you.

From experiments, it seems that git-commit does not seem to allow
empty commit-messages. But I can't find this documented. I'm not sure
what git-commit-tree allows, but I think it should deny creating
commits with empty messages (possibly unless some option is given) if
there should be a restriction, because most scripts use this to
generate commits AFAIK.

But to be honest, it seems to me like in this precise instance it's
probably better to just fix git-rebase--interactive.sh. There's no
good reason for it to barf on the commits -- especially since
noon-interactive rebase handles them just fine. Unless someone screams
out loud, I might take a stab at it when I get time.

-- 
Erik "kusma" Faye-Lund

On Wed, Mar 10, 2010 at 2:34 PM, Erik Faye-Lund
[off-list ref] wrote:

But to be honest, it seems to me like in this precise instance it's
probably better to just fix git-rebase--interactive.sh. There's no
good reason for it to barf on the commits -- especially since
noon-interactive rebase handles them just fine. Unless someone screams
out loud, I might take a stab at it when I get time.

I think I've found the culprit: git-rev-list doesn't append a
newline-separator after commits with empty messages.
git-rebase--interactive.sh basically eats git rev-list's output line
by line, prepending "pick ".

This seems to have been introduced in 55246aa "Don't use "<unknown>"
for placeholders and suppress printing of empty user formats." by
Michal Vitecek. It seems he intended to fix a rev-list with
--pretty=format:"" or something like that, but I can't get custom
formats to work at all with rev-list, even if the documentation says
it should.

Anyway, the following patch seems to fix the problem for me, but I'm
not very confident that it doesn't break whatever Michal was trying to
address.

--->8---

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 5679170..b13e1ba 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c

@@ -134,10 +134,8 @@ static void show_commit(struct commit *commit, void *data)
                                if (graph_show_remainder(revs->graph))
                                        putchar('\n');
                        }
-               } else {
-                       if (buf.len)
-                               printf("%s%c", buf.buf, info->hdr_termination);
-               }
+               } else
+                       printf("%s%c", buf.buf, info->hdr_termination);
                strbuf_release(&buf);
        } else {
                if (graph_show_remainder(revs->graph))
--->8---

-- 
Erik "kusma" Faye-Lund

On Thu, Mar 11, 2010 at 2:14 PM, Erik Faye-Lund
[off-list ref] wrote:

Anyway, the following patch seems to fix the problem for me, but I'm
not very confident that it doesn't break whatever Michal was trying to
address.

...or even the test-suite. I should have tested that before posting
this, sorry for the noise. Back to the drawing-board.

-- 
Erik "kusma" Faye-Lund