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

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

[Erik Faye-Lund]

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

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

[Michael Haggerty]

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

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

[Erik Faye-Lund]

On Wed, Mar 10, 2010 at 2:13 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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 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

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Does git really claim to handle commits with empty commit messages?

Yes, I would say it is a bug in rebase-i if it refuses to deal with
histories with such commits.  Warning and giving the user a chance to
add message _might_ be good, but allowing no way out would be
inexcusable.  Even adding a fake "(original commit had no message)"
as the message and continuing would be better than stopping or
dropping commits.

The Porcelain "git commit" by default complains if you do not give any
message, because it usually is an end-user mistake to directly use
"git commit" and not writing anything about the resulting commit.
However,

 (1) The plumbing allows it because such a policy to forbid commits
     without comments does not belong there, e.g. this would succeed:

     $ git commit-tree HEAD^{tree} </dev/null

 (2) Even the Porcelain allows it if you ask nicely:

     $ git commit --cleanup=verbatim -m ''

These are primarily so that you can deal with histories created by
other people's tools (e.g. foreign SCM, third party Porcelains, etc.).
You do not have much control over histories created by them, and our
commit creating commands need to be usable as an ingredient for you to
write conversion tools (like filter-branch or rebase-i).

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

[Erik Faye-Lund]

On Wed, Mar 10, 2010 at 2:34 PM, Erik Faye-Lund
<kusmabite@googlemail.com> 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

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

[Erik Faye-Lund]

On Thu, Mar 11, 2010 at 2:14 PM, Erik Faye-Lund
<kusmabite@googlemail.com> 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