Re: [PATCH] Add option to git-commit to allow empty log messages

From: Jeff King <peff@peff.net>
Date: Apr 5, 2010, 05:51 UTC
Message-ID: <20100405055139.GA28730@coredump.intra.peff.net>
In-Reply-To: <buor5mu7acd.fsf@dhlpc061.dev.necel.com>

On Mon, Apr 05, 2010 at 02:10:58PM +0900, Miles Bader wrote:

Show 15 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> > As a Porcelain, "git commit" has some leeway to enforce sensible policy on
> > the users, and "forbid commit that does not explain anything" is one such
> > policy.  It is not generally a good idea to expose the full capabilities
> > of plumbing to Porcelain if it leads to bad user behaviour, and such
> > "artificial" limitations are safety features we do not want to remove.
> 
> Isn't the requirement of using a longish option like
> "--allow-empty-message" enough of a warning to users though?
> 
> Although it seems reasonable for git _discourage_ bad practices, I think
> that should generally also be moderated with "... but if you _reallllly_
> want to, you can do this somewhat annoying thing....".  Forcing someone
> to use commit-tree, though, seems a bit much to me; an annoyingly long
> option seems about right.

Yes and no. There are other reasons not to use "git commit" in your import script. You probably want to pass --allow-empty, too, and --no-verify. And you probably want to use --cleanup=none to keep messages intact.

But most of all, even if you do everything right, we still don't promise not to change it out from under you in a future version. Because it's porcelain, and the plumbing method is to use commit-tree. If commit-tree is too hard to use, I would rather see the plumbing made more friendly than encouraging people to build on top of porcelain.

All of that being said, I looked at the snerp-vortex source code (which started this thread):

  http://github.com/rcaputo/snerp-vortex/blob/master/lib/SVN/Dump/Replayer/Git.pm

It uses several pieces of porcelain. Some in silly ways, like calling "git status" to avoid calling git-commit when there are no changes and getting an error code. Which is silly (if you are importing, you probably want --allow-empty), wasteful (you just need the diff-index part of status), and now broken (because status is no longer "commit --dry-run", it always exits with status 0 whether there are changes or not). Then there are things like calling "git add -f" with arguments, and a "TODO: split arguments to handle larger filesets" comment. When he should be using update-index, which takes updates on stdin.

He also notes in the README that it takes 250 seconds to convert his test repo to git, but only 70 to make a flat filesystem, and that he wants to move to using fast-import.

So yes, it sucks that his importer does not support empty comments, and that the OP had to hack around it. But it already doesn't support many things (like commits with a large number of files, and from what I can see, files with spaces will break his `find` invocation). The right answer is for him to move to fast-import, which will be way faster, more robust, and is actually a supported plumbing interface.

I don't think it's worth adding new features to support a scripting interface that we are trying to discourage. And I haven't seen another argument in favor of empty commits besides importing. Are people really wanting to make empty commit messages while using git itself?

-Peff