Does Git really need a commit message to go with a commit?

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Does Git really need a commit message to go with a commit?

Ævar Arnfjörð Bjarmason
git-commit(1) doesn't allow you to make a commit without a commit
message. This is annoying and doesn't properly preserve history in
applications like snerp-vortex which replay a SVN dump into Git. You
have to add `$msg = "Git made me do it" unless length $msg' somewhere.

Is there really no way to add a commit with no message with the git
tools? Will anything break if I manually construct a commit with no
message? Are commit messages inherently part of the format or does
git-commit(1) just think it knows better than me?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: Does Git really need a commit message to go with a commit?

Avery Pennarun
On Sat, Apr 3, 2010 at 6:06 PM, Ævar Arnfjörð Bjarmason
<[hidden email]> wrote:
> git-commit(1) doesn't allow you to make a commit without a commit
> message. This is annoying and doesn't properly preserve history in
> applications like snerp-vortex which replay a SVN dump into Git. You
> have to add `$msg = "Git made me do it" unless length $msg' somewhere.
>
> Is there really no way to add a commit with no message with the git
> tools? Will anything break if I manually construct a commit with no
> message? Are commit messages inherently part of the format or does
> git-commit(1) just think it knows better than me?

git-commit isn't really meant to be used by scripts.  Try using
git-commit-tree instead, which doesn't enforce a commit message.

(Or use git fast-import; a quick glance at Snerp suggests that it's
intended to be *fast*; using fast-import ought to make things vastly
quicker.)

Avery
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: Does Git really need a commit message to go with a commit?

Santi Béjar-2
In reply to this post by Ævar Arnfjörð Bjarmason
On Sun, Apr 4, 2010 at 1:15 PM, Ævar Arnfjörð Bjarmason
<[hidden email]> wrote:

> On Sat, Apr 3, 2010 at 22:26, Santi Béjar <[hidden email]> wrote:
>> On Sun, Apr 4, 2010 at 12:06 AM, Ævar Arnfjörð Bjarmason
>> <[hidden email]> wrote:
>>> git-commit(1) doesn't allow you to make a commit without a commit
>>> message. This is annoying and doesn't properly preserve history in
>>> applications like snerp-vortex which replay a SVN dump into Git. You
>>> have to add `$msg = "Git made me do it" unless length $msg' somewhere.
>>>
>>> Is there really no way to add a commit with no message with the git
>>> tools? Will anything break if I manually construct a commit with no
>>> message? Are commit messages inherently part of the format or does
>>> git-commit(1) just think it knows better than me?
>>
>> Normally it does not make sense an empty commit message, so it is
>> forbidden by default. But there is a flag (documented in the man page)
>> since v1.5.4 (v1.5.3.7-994-g36863af git-commit --allow-empty,
>> 2007-12-03) to allow it.
>
> Actually --allow-empty allows you to commit an empty /tree/. It
> doesn't do anything for the commit message itself.

Arg! You are right, sorry for the noise. I should not write emails
during the night...

I then suppose your only option is to use plumbing commands (see
git(1)), in this case git-commit-tree. In general if you write scripts
around git you should use only plumbing commands as they don't change
the behavior.

HTH,
Santi
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ævar Arnfjörð Bjarmason
In reply to this post by Ævar Arnfjörð Bjarmason
Change git-commit(1) to accept a --allow-empty-message option
analogous to the existing --allow-empty option which allows empty
trees. This is mainly for compatability with foreign SCM systems.

Signed-off-by: Ævar Arnfjörð Bjarmason <[hidden email]>
---
 Documentation/git-commit.txt          |   12 +++++++--
 builtin/commit.c                      |    7 +++--
 t/t7510-commit-allow-empty-message.sh |   41 +++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 6 deletions(-)
 create mode 100755 t/t7510-commit-allow-empty-message.sh

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 64fb458..2c1c2e1 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -10,9 +10,9 @@ SYNOPSIS
 [verse]
 'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
    [(-c | -C) <commit>] [-F <file> | -m <msg>] [--reset-author]
-   [--allow-empty] [--no-verify] [-e] [--author=<author>]
-   [--date=<date>] [--cleanup=<mode>] [--status | --no-status] [--]
-   [[-i | -o ]<file>...]
+   [--allow-empty] [--allow-empty-message] [--no-verify] [-e]
+   [--author=<author>] [--date=<date>] [--cleanup=<mode>]
+   [--status | --no-status] [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -131,6 +131,12 @@ OPTIONS
  from making such a commit.  This option bypasses the safety, and
  is primarily for use by foreign scm interface scripts.
 
+--allow-empty-message::
+ Like --allow-empty this command is primarily for use by foreign
+ scm interface scripts. It allows you to create a commit with an
+ empty commit message without using plumbing commands like
+ linkgit:git-commit-tree[1].
+
 --cleanup=<mode>::
  This option sets how the commit message is cleaned up.
  The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
diff --git a/builtin/commit.c b/builtin/commit.c
index c5ab683..7fd963e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -65,8 +65,8 @@ static const char *template_file;
 static char *edit_message, *use_message;
 static char *author_name, *author_email, *author_date;
 static int all, edit_flag, also, interactive, only, amend, signoff;
-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
-static int no_post_rewrite;
+static int quiet, verbose, no_verify, allow_empty, allow_empty_message, dry_run;
+static int no_post_rewrite, renew_authorship;
 static char *untracked_files_arg, *force_date;
 /*
  * The default commit message cleanup mode will remove the lines
@@ -141,6 +141,7 @@ static struct option builtin_commit_options[] = {
  OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
  { OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
  OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
+ OPT_BOOLEAN(0, "allow-empty-message", &allow_empty_message, "ok to record a change with an empty message"),
  /* end commit contents options */
 
  OPT_END()
@@ -1293,7 +1294,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
  if (cleanup_mode != CLEANUP_NONE)
  stripspace(&sb, cleanup_mode == CLEANUP_ALL);
- if (message_is_empty(&sb)) {
+ if (message_is_empty(&sb) && !allow_empty_message) {
  rollback_index_files();
  fprintf(stderr, "Aborting commit due to empty commit message.\n");
  exit(1);
diff --git a/t/t7510-commit-allow-empty-message.sh b/t/t7510-commit-allow-empty-message.sh
new file mode 100755
index 0000000..d7dc0da
--- /dev/null
+++ b/t/t7510-commit-allow-empty-message.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Ævar Arnfjörð Bjarmason
+#
+
+test_description='git commit --allow-empty-message'
+
+. ./test-lib.sh
+
+commit_msg_is () {
+ test "`git log --pretty=format:%s%b -1`" = "$1"
+}
+
+# A sanity check to see if commit is working at all.
+test_expect_success 'a basic commit in an empty tree should succeed' '
+ (
+ echo content > foo &&
+ git add foo &&
+ git commit -m "initial commit"
+ ) &&
+ commit_msg_is "initial commit"
+'
+
+test_expect_success 'Commit no message with --allow-empty-message' '
+ (
+ echo "more content" >> foo &&
+ git add foo &&
+ printf "" | git commit --allow-empty-message
+ ) &&
+ commit_msg_is ""
+'
+
+test_expect_success 'Commit a message with --allow-empty-message' '
+ (
+ echo "even more content" >> foo &&
+ git add foo &&
+ git commit --allow-empty-message -m"hello there"
+ ) &&
+ commit_msg_is "hello there"
+'
+test_done
--
1.7.0.4.298.gc81d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

David Aguilar
On Sun, Apr 04, 2010 at 02:49:16PM +0000, Ævar Arnfjörð Bjarmason wrote:
> Change git-commit(1) to accept a --allow-empty-message option
> analogous to the existing --allow-empty option which allows empty
> trees. This is mainly for compatability with foreign SCM systems.

It's hard enough to get co-workers to write good commit
messages.  I wouldn't want to encourage them to skip writing
them altogether (by the existence of this feature).

Is there any reason why the git commit-tree plumbing didn't
suffice?

(in other words, "sure we can", but I'm asking,
 "are you sure we should?")

Just my $.02.
Hey, we just had a small earthquake.  Fun =)


> Signed-off-by: Ævar Arnfjörð Bjarmason <[hidden email]>
> ---
>  Documentation/git-commit.txt          |   12 +++++++--
>  builtin/commit.c                      |    7 +++--
>  t/t7510-commit-allow-empty-message.sh |   41 +++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 6 deletions(-)
>  create mode 100755 t/t7510-commit-allow-empty-message.sh
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 64fb458..2c1c2e1 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -10,9 +10,9 @@ SYNOPSIS
>  [verse]
>  'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
>     [(-c | -C) <commit>] [-F <file> | -m <msg>] [--reset-author]
> -   [--allow-empty] [--no-verify] [-e] [--author=<author>]
> -   [--date=<date>] [--cleanup=<mode>] [--status | --no-status] [--]
> -   [[-i | -o ]<file>...]
> +   [--allow-empty] [--allow-empty-message] [--no-verify] [-e]
> +   [--author=<author>] [--date=<date>] [--cleanup=<mode>]
> +   [--status | --no-status] [--] [[-i | -o ]<file>...]
>  
>  DESCRIPTION
>  -----------
> @@ -131,6 +131,12 @@ OPTIONS
>   from making such a commit.  This option bypasses the safety, and
>   is primarily for use by foreign scm interface scripts.
>  
> +--allow-empty-message::
> + Like --allow-empty this command is primarily for use by foreign
> + scm interface scripts. It allows you to create a commit with an
> + empty commit message without using plumbing commands like
> + linkgit:git-commit-tree[1].
> +
>  --cleanup=<mode>::
>   This option sets how the commit message is cleaned up.
>   The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
> diff --git a/builtin/commit.c b/builtin/commit.c
> index c5ab683..7fd963e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -65,8 +65,8 @@ static const char *template_file;
>  static char *edit_message, *use_message;
>  static char *author_name, *author_email, *author_date;
>  static int all, edit_flag, also, interactive, only, amend, signoff;
> -static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
> -static int no_post_rewrite;
> +static int quiet, verbose, no_verify, allow_empty, allow_empty_message, dry_run;
> +static int no_post_rewrite, renew_authorship;
>  static char *untracked_files_arg, *force_date;
>  /*
>   * The default commit message cleanup mode will remove the lines
> @@ -141,6 +141,7 @@ static struct option builtin_commit_options[] = {
>   OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
>   { OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
>   OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
> + OPT_BOOLEAN(0, "allow-empty-message", &allow_empty_message, "ok to record a change with an empty message"),
>   /* end commit contents options */
>  
>   OPT_END()
> @@ -1293,7 +1294,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  
>   if (cleanup_mode != CLEANUP_NONE)
>   stripspace(&sb, cleanup_mode == CLEANUP_ALL);
> - if (message_is_empty(&sb)) {
> + if (message_is_empty(&sb) && !allow_empty_message) {
>   rollback_index_files();
>   fprintf(stderr, "Aborting commit due to empty commit message.\n");
>   exit(1);
> diff --git a/t/t7510-commit-allow-empty-message.sh b/t/t7510-commit-allow-empty-message.sh
> new file mode 100755
> index 0000000..d7dc0da
> --- /dev/null
> +++ b/t/t7510-commit-allow-empty-message.sh
> @@ -0,0 +1,41 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2010 Ævar Arnfjörð Bjarmason
> +#
> +
> +test_description='git commit --allow-empty-message'
> +
> +. ./test-lib.sh
> +
> +commit_msg_is () {
> + test "`git log --pretty=format:%s%b -1`" = "$1"
> +}
> +
> +# A sanity check to see if commit is working at all.
> +test_expect_success 'a basic commit in an empty tree should succeed' '
> + (
> + echo content > foo &&
> + git add foo &&
> + git commit -m "initial commit"
> + ) &&
> + commit_msg_is "initial commit"
> +'
> +
> +test_expect_success 'Commit no message with --allow-empty-message' '
> + (
> + echo "more content" >> foo &&
> + git add foo &&
> + printf "" | git commit --allow-empty-message
> + ) &&
> + commit_msg_is ""
> +'
> +
> +test_expect_success 'Commit a message with --allow-empty-message' '
> + (
> + echo "even more content" >> foo &&
> + git add foo &&
> + git commit --allow-empty-message -m"hello there"
> + ) &&
> + commit_msg_is "hello there"
> +'
> +test_done
> --
> 1.7.0.4.298.gc81d
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
                David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ævar Arnfjörð Bjarmason
In retrospect I probably should have sent a [PATCH RFC], this is
obviously somewhat of a delicate subject.

On Sun, Apr 4, 2010 at 21:21, Shawn Pearce <[hidden email]> wrote:
> Isn't this exactly why git-commit-tree exists?  I don't really see a reason
> to support scripting the porcelain git commit.

It's not so much about supporting scripting as exporting the
capabilities of the porcelain to the frontend utilities without
artificial limitations.

On Sun, Apr 4, 2010 at 22:43, David Aguilar <[hidden email]> wrote:
> On Sun, Apr 04, 2010 at 02:49:16PM +0000, Ævar Arnfjörð Bjarmason wrote:
>> Change git-commit(1) to accept a --allow-empty-message option
>> analogous to the existing --allow-empty option which allows empty
>> trees. This is mainly for compatability with foreign SCM systems.
>
> Is there any reason why the git commit-tree plumbing didn't
> suffice?

I encountered this limitation most recently while using hacking
snerp-vortex which uses git-commit(1) directly. While it should
ideally use git-commit-tree(1) or git-fast-import(1) it was easier at
the time to do:

    message ||= "Git made me";

I thought it was silly that I had to either do that to fix an
otherwise working piece of software or rewrite how it does commits,
hence the patch.

> It's hard enough to get co-workers to write good commit
> messages.  I wouldn't want to encourage them to skip writing
> them altogether (by the existence of this feature).

I'm a big fan of good commit messages, that's another reason why I
think this feature is a good idea.

I've read too many commit messages from people who're obviously
determined not to write any, but are being forced to do so by their
tools. Favorites include "more updates", "some changes", "..." (the
list goes on). I'd rather get nothing at all from those people than
more meaningless drivel. It would increase the signal-to-noise ratio
of git-log(1) output.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Ævar Arnfjörð Bjarmason <[hidden email]> writes:

> It's not so much about supporting scripting as exporting the
> capabilities of the porcelain to the frontend utilities without
> artificial limitations.

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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Sverre Rabbelier-2
Heya,

On Sun, Apr 4, 2010 at 21:11, Junio C Hamano <[hidden email]> wrote:
> 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.

You contradict yourself:

commit 5241b6bfe2285a6da598a0348c37b77964035bc8
Author: Junio C Hamano <[hidden email]>
Date:   Mon Dec 3 00:03:10 2007 -0800

    git-commit --allow-empty

    It does not usually make sense to record a commit that has the exact
    same tree as its sole parent commit and that is why git-commit prevents
    you from making such a mistake, but when data from foreign scm is
    involved, it is a different story.  We are equipped to represent such an
    (perhaps insane, perhaps by mistake, or perhaps done on purpose) empty
    change, and it is better to represent it bypassing the safety valve for
    native use.

    This is primarily for use by foreign scm interface scripts.

    Signed-off-by: Junio C Hamano <[hidden email]>


--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Sverre Rabbelier <[hidden email]> writes:

> On Sun, Apr 4, 2010 at 21:11, Junio C Hamano <[hidden email]> wrote:
>> 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.
>
> You contradict yourself:

I personally don't mind very much removing --allow-empty, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Miles Bader-2
In reply to this post by Junio C Hamano
Junio C Hamano <[hidden email]> 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.

-Miles

--
Love is the difficult realization that something other than oneself is real.
[Iris Murdoch]
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Miles Bader <[hidden email]> writes:

> Junio C Hamano <[hidden email]> 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?

Yes, but re-read the part you omitted from your quote, where Ævar makes it
sound as if exposing plumbing's flexibility to the Porcelain layer is
unconditionally a good thing.  My point is it never is.

And as you said (and as Sverre alluded to with his --allow-empty), longish
option is one way to ensure that we do not unconditionally expose
flexibility from the plumbing without thinking.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
In reply to this post by Miles Bader-2
On Mon, Apr 05, 2010 at 02:10:58PM +0900, Miles Bader wrote:

> Junio C Hamano <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ævar Arnfjörð Bjarmason
On Mon, Apr 5, 2010 at 05:51, Jeff King <[hidden email]> wrote:
> 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.

Thanks for looking at the importer, that'll be very useful when fixing
it. But FWIW that `find` invocation isn't a bug. Perl doesn't have a "
" input field separator so "my @files = `find . -type f`" does the
right thing, unlike in the shell.

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

I'm sorry that I brought snerp-vortex into this at all. It wasn't the
main motivation behind this patch, just the straw that broke the
camel's back.

I've run into this limitation a lot when playing with and learning
Git. Sometimes I'm e.g. making small throwaway repositories in /tmp
using the porcelain for  experimentation. Those have seen a lot of
"blah!" commit messages immediately following "Git exited abnormally
with code 1".

Miles Bader said it better than I could. Tools should provide sane
defaults and discourage bad practices, but they shouldn't *enforce*
good practices. That'll inevitably burn people whose use for the tools
isn't what you expect.

Even if they Git maintainers don't like this people *do* write
automated scripts and wrappers around Git using the porcelain, simply
because that's what they're used to. Learning to use and switching to
something like git-fast-import(1) or git-commit-tree(1) is too big of
a hurdle for small throwaway scripts that take ~1m to write and aren't
big dedicated importers like snerp-vortex.

There's probably a lot of code out there doing `git commit -m"Yet
another revision"' from some cron job. I sprinkled a lot of such
meaningless messages about when I switched from Subversion (which
supports empty commits in the porcelain) for these automated jobs to
Git.

Of course Junio may disagree (and that's fine!), how much you babysit
your users is ultimately a design decision up to the maintainer. I
just find this inconsistent with the rest of the porcelain which
usually gives me plenty of ammo to blow both my legs of (and the
planet they were standing on) if I so choose :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Sverre Rabbelier-2
In reply to this post by Junio C Hamano
Heya,

On Sun, Apr 4, 2010 at 22:57, Junio C Hamano <[hidden email]> wrote:
> Sverre Rabbelier <[hidden email]> writes:
>> You contradict yourself:
>
> I personally don't mind very much removing --allow-empty, though.

Ah, hmm. I vaguely remember some discussion in the past about empty
commits, but I'm not sure what the conclusion there was? I think it
was something like "if you want to create a marker, use tags instead,
there's really no reason to be using --allow-empty". Wasn't there some
use case for "--allow-emtpy" when creating an unrooted branch with an
empty starting point, or somesuch?

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jonathan Nieder-2
In reply to this post by Ævar Arnfjörð Bjarmason
Hi,

Ævar Arnfjörð Bjarmason wrote:

> Learning to use and switching to
> something like git-fast-import(1) or git-commit-tree(1) is too big of
> a hurdle for small throwaway scripts that take ~1m to write and aren't
> big dedicated importers like snerp-vortex.
>
> There's probably a lot of code out there doing `git commit -m"Yet
> another revision"' from some cron job.

FWIW, I have no strong opinion about whether to add this --allow-empty-message
option.  Maybe it would make something more convenient for someone,
though that has to be weighed against it making it harder for everyone
else to read the manual.

Instead, I just wanted to suggest that it is really worth the time to
learn to use ‘git commit-tree’.  Mostly because it does not take much
time to learn at all.

Hint:

        parent=HEAD && : or whatever &&
        tree=$(git write-tree) &&
        printf "%s\n" message |
        commit=$(git commit-tree "$tree" -p "$parent") &&
        git update-ref refs/heads/somebranch "$commit"

It is a very flexible tool, and I think you will find yourself fighting
with git much less if you use tools like it in situations that would
be unusual for a human to run into.

Cheers,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jonathan Nieder-2
Jonathan Nieder wrote:

> parent=HEAD && : or whatever &&
> tree=$(git write-tree) &&
> printf "%s\n" message |
> commit=$(git commit-tree "$tree" -p "$parent") &&
> git update-ref refs/heads/somebranch "$commit"

I rearranged this example the last minute and broke it.  You might
say this proves the opposite of my point, though I don’t think that
would be warranted.  Anyway, to feed the message into ‘git
commit-tree’ standard input, the relevant part of the example should
have read as follows:

commit=$(
        printf "%s\n" message |
        git commit-tree "$tree" -p "$parent"
)

Incidentally, outside of the user manual and contrib/examples/ in the
sources, the git documentation does not have many examples like this
to point to, which is too bad.

Sorry for the confusion,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
In reply to this post by Ævar Arnfjörð Bjarmason
On Mon, Apr 05, 2010 at 12:50:11PM +0000, Ævar Arnfjörð Bjarmason wrote:

> Thanks for looking at the importer, that'll be very useful when fixing
> it. But FWIW that `find` invocation isn't a bug. Perl doesn't have a "
> " input field separator so "my @files = `find . -type f`" does the
> right thing, unlike in the shell.

He calls `find $dir -type f`, without a quotemeta on $dir, which perl
will pass to the shell to interpret (and is actually a security issue if
I can convince you to try importing my svn repository with directory ';
rm -rf /;').

> I'm sorry that I brought snerp-vortex into this at all. It wasn't the
> main motivation behind this patch, just the straw that broke the
> camel's back.

I don't in particular have a problem with --allow-empty-message for
casual use. Why anybody would want to use it when they could type the
much shorter -mnone, I don't know. But it is long enough that people
will have to think about using it, which means the people who do so will
probably really want it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
In reply to this post by Jonathan Nieder-2
On Mon, Apr 05, 2010 at 12:58:22PM -0500, Jonathan Nieder wrote:

> > There's probably a lot of code out there doing `git commit -m"Yet
> > another revision"' from some cron job.
>
> FWIW, I have no strong opinion about whether to add this --allow-empty-message
> option.  Maybe it would make something more convenient for someone,
> though that has to be weighed against it making it harder for everyone
> else to read the manual.

I meant to mention this in my other response: I would prefer if such an
option doesn't clutter up the usage message. --allow-empty is already
there, and probably doesn't need to be. "git commit -h 2>&1 | wc -l"
shows a whopping 39 lines, which IMHO is too many for a short usage
summary. I mean, "--no-post-rewrite", is that really one of the top-used
options?

> Hint:
>
> parent=HEAD && : or whatever &&
> tree=$(git write-tree) &&
> printf "%s\n" message |
> commit=$(git commit-tree "$tree" -p "$parent") &&
> git update-ref refs/heads/somebranch "$commit"

In addition to the bug you mention later, you also probably want to do:

  parent=`git rev-parse --verify HEAD`
  [...]
  git update-ref \
    -m 'automated commit by tool X' \
    refs/heads/somebranch $commit $parent

which will give your reflog entry a more useful message, and will
protect against simultaneous updates losing history (update-ref will
make sure, while locked, that somebranch contains the $parent sha1 you
wrote as part of the commit object).

And of course it still doesn't handle parentless root commits.

So doing it right really is a bit more work than just calling "git
commit".

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ævar Arnfjörð Bjarmason
Change git-commit(1) to accept a --allow-empty-message option
analogous to the existing --allow-empty option which allows empty
trees. This is mainly for compatability with foreign SCM systems.

Signed-off-by: Ævar Arnfjörð Bjarmason <[hidden email]>
---
 Documentation/git-commit.txt          |    6 +++++
 builtin/commit.c                      |    7 +++--
 t/t7510-commit-allow-empty-message.sh |   41 +++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 3 deletions(-)
 create mode 100755 t/t7510-commit-allow-empty-message.sh

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 64fb458..aa18bee 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -131,6 +131,12 @@ OPTIONS
  from making such a commit.  This option bypasses the safety, and
  is primarily for use by foreign scm interface scripts.
 
+--allow-empty-message::
+       Like --allow-empty this command is primarily for use by foreign
+       scm interface scripts. It allows you to create a commit with an
+       empty commit message without using plumbing commands like
+       linkgit:git-commit-tree[1].
+
 --cleanup=<mode>::
  This option sets how the commit message is cleaned up.
  The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
diff --git a/builtin/commit.c b/builtin/commit.c
index c5ab683..7fd963e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -65,8 +65,8 @@ static const char *template_file;
 static char *edit_message, *use_message;
 static char *author_name, *author_email, *author_date;
 static int all, edit_flag, also, interactive, only, amend, signoff;
-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
-static int no_post_rewrite;
+static int quiet, verbose, no_verify, allow_empty, allow_empty_message, dry_run;
+static int no_post_rewrite, renew_authorship;
 static char *untracked_files_arg, *force_date;
 /*
  * The default commit message cleanup mode will remove the lines
@@ -141,6 +141,7 @@ static struct option builtin_commit_options[] = {
  OPT_BOOLEAN(0, "no-post-rewrite", &no_post_rewrite, "bypass post-rewrite hook"),
  { OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, "mode", "show untracked files, optional modes: all, normal, no. (Default: all)", PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
  OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
+ OPT_BOOLEAN(0, "allow-empty-message", &allow_empty_message, "ok to record a change with an empty message"),
  /* end commit contents options */
 
  OPT_END()
@@ -1293,7 +1294,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
  if (cleanup_mode != CLEANUP_NONE)
  stripspace(&sb, cleanup_mode == CLEANUP_ALL);
- if (message_is_empty(&sb)) {
+ if (message_is_empty(&sb) && !allow_empty_message) {
  rollback_index_files();
  fprintf(stderr, "Aborting commit due to empty commit message.\n");
  exit(1);
diff --git a/t/t7510-commit-allow-empty-message.sh b/t/t7510-commit-allow-empty-message.sh
new file mode 100755
index 0000000..d7dc0da
--- /dev/null
+++ b/t/t7510-commit-allow-empty-message.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Ævar Arnfjörð Bjarmason
+#
+
+test_description='git commit --allow-empty-message'
+
+. ./test-lib.sh
+
+commit_msg_is () {
+ test "`git log --pretty=format:%s%b -1`" = "$1"
+}
+
+# A sanity check to see if commit is working at all.
+test_expect_success 'a basic commit in an empty tree should succeed' '
+ (
+ echo content > foo &&
+ git add foo &&
+ git commit -m "initial commit"
+ ) &&
+ commit_msg_is "initial commit"
+'
+
+test_expect_success 'Commit no message with --allow-empty-message' '
+ (
+ echo "more content" >> foo &&
+ git add foo &&
+ printf "" | git commit --allow-empty-message
+ ) &&
+ commit_msg_is ""
+'
+
+test_expect_success 'Commit a message with --allow-empty-message' '
+ (
+ echo "even more content" >> foo &&
+ git add foo &&
+ git commit --allow-empty-message -m"hello there"
+ ) &&
+ commit_msg_is "hello there"
+'
+test_done
--
1.7.0.4.298.gc81d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Ævar Arnfjörð Bjarmason
In reply to this post by Jeff King
On Tue, Apr 6, 2010 at 05:55, Jeff King <[hidden email]> wrote:

> On Mon, Apr 05, 2010 at 12:58:22PM -0500, Jonathan Nieder wrote:
>
>> > There's probably a lot of code out there doing `git commit -m"Yet
>> > another revision"' from some cron job.
>>
>> FWIW, I have no strong opinion about whether to add this --allow-empty-message
>> option.  Maybe it would make something more convenient for someone,
>> though that has to be weighed against it making it harder for everyone
>> else to read the manual.
>
> I meant to mention this in my other response: I would prefer if such an
> option doesn't clutter up the usage message. --allow-empty is already
> there, and probably doesn't need to be. "git commit -h 2>&1 | wc -l"
> shows a whopping 39 lines, which IMHO is too many for a short usage
> summary. I mean, "--no-post-rewrite", is that really one of the top-used
> options?

I just put it there because --allow-empty was there and I thought the
SYNOPSIS might be going for a complete listing. I completely agree
though, the option shouldn't be in the SYNOPSIS, and neither should
--allow-empty be. They're both analogous to obscure options like
--porcelain (which isn't listed).

I've just submitted a new patch to rectify this. I can send another
one to remove --allow-empty from the SYNOPSIS.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
12