All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: empty ident name trashes commit message
       [not found] <CAMej=25=xj61pc+k42dv3byuBqUJzW21Sz+BXwoufqnKwV5Bbg@mail.gmail.com>
@ 2012-07-21 14:26 ` Ramana Kumar
  2012-07-23 17:27   ` Jeff King
  0 siblings, 1 reply; 16+ messages in thread
From: Ramana Kumar @ 2012-07-21 14:26 UTC (permalink / raw)
  To: git

If I forget to set user.email and user.name config options and do a commit
(possibly the --amend option also required to make this show up), then git
1.7.11.2 will drops me into an editor for a commit message, then after that
complain with the fatal message:

   *** Please tell me who you are.

   Run

     git config --global user.email "you@example.com"
     git config --global user.name "Your Name"

   to set your account's default identity.
   Omit --global to set the identity only in this repository.

   fatal: empty ident name (for <ramana.kumar@gmail.com>) not allowed

The commit message I wrote is now lost. This is bad behaviour - the error
should happen before one writes the commit message, or the message should be
saved somewhere.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: empty ident name trashes commit message
  2012-07-21 14:26 ` empty ident name trashes commit message Ramana Kumar
@ 2012-07-23 17:27   ` Jeff King
  2012-07-23 18:05     ` Ramana Kumar
  2012-07-23 18:46     ` Jeff King
  0 siblings, 2 replies; 16+ messages in thread
From: Jeff King @ 2012-07-23 17:27 UTC (permalink / raw)
  To: Ramana Kumar; +Cc: git

On Sat, Jul 21, 2012 at 03:26:26PM +0100, Ramana Kumar wrote:

> If I forget to set user.email and user.name config options and do a commit
> (possibly the --amend option also required to make this show up), then git
> 1.7.11.2 will drops me into an editor for a commit message, then after that
> complain with the fatal message:
> 
>    *** Please tell me who you are.
> [...]

Hmm. I think this is an artifact of running --amend. In the normal case,
we check the author ident beforehand. But in the --amend case, we take
the existing author, but then fail trying to generate the committer
ident. So we could probably do better by checking both explicitly
beforehand.

>    fatal: empty ident name (for <ramana.kumar@gmail.com>) not allowed

Usually we would fall back to your name from /etc/passwd. I guess it is
blank on your system.

> The commit message I wrote is now lost. This is bad behaviour - the error
> should happen before one writes the commit message, or the message should be
> saved somewhere.

It's not lost. It's in .git/COMMIT_EDITMSG.

We could probably do a better job of informing the user of this when
commit dies prematurely.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: empty ident name trashes commit message
  2012-07-23 17:27   ` Jeff King
@ 2012-07-23 18:05     ` Ramana Kumar
  2012-07-23 18:46     ` Jeff King
  1 sibling, 0 replies; 16+ messages in thread
From: Ramana Kumar @ 2012-07-23 18:05 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Mon, Jul 23, 2012 at 6:27 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Jul 21, 2012 at 03:26:26PM +0100, Ramana Kumar wrote:
>
>> If I forget to set user.email and user.name config options and do a commit
>> (possibly the --amend option also required to make this show up), then git
>> 1.7.11.2 will drops me into an editor for a commit message, then after that
>> complain with the fatal message:
>>
>>    *** Please tell me who you are.
>> [...]
>
> Hmm. I think this is an artifact of running --amend. In the normal case,
> we check the author ident beforehand. But in the --amend case, we take
> the existing author, but then fail trying to generate the committer
> ident. So we could probably do better by checking both explicitly
> beforehand.

Indeed.

> Usually we would fall back to your name from /etc/passwd. I guess it is
> blank on your system.
>
>> The commit message I wrote is now lost. [...]
>
> It's not lost. It's in .git/COMMIT_EDITMSG.
>
> We could probably do a better job of informing the user of this when
> commit dies prematurely.
>
> -Peff

I agree, and thank you very much for those two useful pieces of
information! (names stored in /etc/passwd and saving of
.git/COMMIT_EDITMSG).

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: empty ident name trashes commit message
  2012-07-23 17:27   ` Jeff King
  2012-07-23 18:05     ` Ramana Kumar
@ 2012-07-23 18:46     ` Jeff King
  2012-07-23 18:48       ` [PATCH 1/3] advice: pass varargs to strbuf_vaddf, not strbuf_addf Jeff King
                         ` (2 more replies)
  1 sibling, 3 replies; 16+ messages in thread
From: Jeff King @ 2012-07-23 18:46 UTC (permalink / raw)
  To: Ramana Kumar; +Cc: Junio C Hamano, git

On Mon, Jul 23, 2012 at 01:27:26PM -0400, Jeff King wrote:

> On Sat, Jul 21, 2012 at 03:26:26PM +0100, Ramana Kumar wrote:
> 
> > If I forget to set user.email and user.name config options and do a commit
> > (possibly the --amend option also required to make this show up), then git
> > 1.7.11.2 will drops me into an editor for a commit message, then after that
> > complain with the fatal message:
> > 
> >    *** Please tell me who you are.
> > [...]
> 
> Hmm. I think this is an artifact of running --amend. In the normal case,
> we check the author ident beforehand. But in the --amend case, we take
> the existing author, but then fail trying to generate the committer
> ident. So we could probably do better by checking both explicitly
> beforehand.

It seems we already had such a check, but it was not done correctly.
This is fixed by patch 2 below.

> > The commit message I wrote is now lost. This is bad behaviour - the error
> > should happen before one writes the commit message, or the message should be
> > saved somewhere.
> 
> It's not lost. It's in .git/COMMIT_EDITMSG.
> 
> We could probably do a better job of informing the user of this when
> commit dies prematurely.

And patch 3 improves this situation.

  [1/3]: advice: pass varargs to strbuf_vaddf, not strbuf_addf
  [2/3]: commit: check committer identity more strictly
  [3/3]: commit: give a hint when a commit message has been abandoned

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/3] advice: pass varargs to strbuf_vaddf, not strbuf_addf
  2012-07-23 18:46     ` Jeff King
@ 2012-07-23 18:48       ` Jeff King
  2012-07-23 18:50       ` [PATCH 2/3] commit: check committer identity more strictly Jeff King
  2012-07-23 18:52       ` [PATCH 3/3] commit: give a hint when a commit message has been abandoned Jeff King
  2 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2012-07-23 18:48 UTC (permalink / raw)
  To: Ramana Kumar; +Cc: Junio C Hamano, git

The advise() function takes a variable number of arguments
and converts them into a va_list object to pass to strbuf
for handling. However, we accidentally called strbuf_addf
(that takes a variable number of arguments) instead of
strbuf_vaddf (that takes a va_list).

This bug dates back to v1.7.8.1-1-g23cb5bf, but we never
noticed because none of the current callers passes a string
with a format specifier in it. And the compiler did not
notice because the format string is not available at
compile time.

Signed-off-by: Jeff King <peff@peff.net>
---
 advice.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/advice.c b/advice.c
index a492eea..edfbd4a 100644
--- a/advice.c
+++ b/advice.c
@@ -32,7 +32,7 @@ void advise(const char *advice, ...)
 	const char *cp, *np;
 
 	va_start(params, advice);
-	strbuf_addf(&buf, advice, params);
+	strbuf_vaddf(&buf, advice, params);
 	va_end(params);
 
 	for (cp = buf.buf; *cp; cp = np) {
-- 
1.7.10.5.40.g059818d

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] commit: check committer identity more strictly
  2012-07-23 18:46     ` Jeff King
  2012-07-23 18:48       ` [PATCH 1/3] advice: pass varargs to strbuf_vaddf, not strbuf_addf Jeff King
@ 2012-07-23 18:50       ` Jeff King
  2012-07-23 20:51         ` Junio C Hamano
  2012-07-23 18:52       ` [PATCH 3/3] commit: give a hint when a commit message has been abandoned Jeff King
  2 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2012-07-23 18:50 UTC (permalink / raw)
  To: Ramana Kumar; +Cc: Junio C Hamano, git

The identity of the committer will ultimately be pulled from
the ident code by commit_tree(). However, we make an attempt
to check the author and committer identity early, before the
user has done any manual work like inputting a commit
message. That lets us abort without them having to worry
about salvaging the work from .git/COMMIT_EDITMSG.

The early check for committer ident does not use the
IDENT_STRICT flag, meaning that it would not find an empty
name field. The motivation was presumably because we did not
want to be too restrictive, as later calls might be more lax
(for example, when we create the reflog entry, we do not
care too much about a real name). However, because
commit_tree will always get a strict identity to put in the
commit object itself, there is no point in being lax only to
die later (and in fact it is harmful, because the user will
have wasted time typing their commit message).

Incidentally, this bug was masked prior to 060d4bb, as the
initial loose call would taint the later strict call. So the
commit would succeed (albeit with a bogus committer line in
the commit object), and nobody noticed that our early check
did not match the later one.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 95eeab1..20cef95 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -725,7 +725,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
-	strbuf_addstr(&committer_ident, git_committer_info(0));
+	strbuf_addstr(&committer_ident, git_committer_info(IDENT_STRICT));
 	if (use_editor && include_status) {
 		char *ai_tmp, *ci_tmp;
 		if (whence != FROM_COMMIT)
-- 
1.7.10.5.40.g059818d

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 18:46     ` Jeff King
  2012-07-23 18:48       ` [PATCH 1/3] advice: pass varargs to strbuf_vaddf, not strbuf_addf Jeff King
  2012-07-23 18:50       ` [PATCH 2/3] commit: check committer identity more strictly Jeff King
@ 2012-07-23 18:52       ` Jeff King
  2012-07-23 20:49         ` Junio C Hamano
  2 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2012-07-23 18:52 UTC (permalink / raw)
  To: Ramana Kumar; +Cc: Junio C Hamano, git

If we launch an editor for the user to create a commit
message, they may put significant work into doing so.
Typically we try to check common mistakes that could cause
the commit to fail early, so that we die before the user
goes to the trouble.

We may still experience some errors afterwards, though; in
this case, the user is given no hint that their commit
message has been saved. Let's tell them where it is.

Signed-off-by: Jeff King <peff@peff.net>
---
I did not bother protecting this with advice.* config, as it is unlikely
to come up regularly. If somebody cares, they are welcome to add it on
top.

 builtin/commit.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 20cef95..149e07d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -116,6 +116,16 @@ static enum {
 	STATUS_FORMAT_PORCELAIN
 } status_format = STATUS_FORMAT_LONG;
 
+static int mention_abandoned_message;
+static void maybe_mention_abandoned_message(void)
+{
+	if (!mention_abandoned_message)
+		return;
+	advise(_("Your commit message has been saved in '%s' and will be\n"
+		 "overwritten by the next invocation of \"git commit\"."),
+	       git_path(commit_editmsg));
+}
+
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
 	struct strbuf *buf = opt->value;
@@ -848,6 +858,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
 		}
+		atexit(maybe_mention_abandoned_message);
+		mention_abandoned_message = 1;
 	}
 
 	if (!no_verify &&
@@ -1532,11 +1544,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (template_untouched(&sb) && !allow_empty_message) {
 		rollback_index_files();
 		fprintf(stderr, _("Aborting commit; you did not edit the message.\n"));
+		mention_abandoned_message = 0;
 		exit(1);
 	}
 	if (message_is_empty(&sb) && !allow_empty_message) {
 		rollback_index_files();
 		fprintf(stderr, _("Aborting commit due to empty commit message.\n"));
+		mention_abandoned_message = 0;
 		exit(1);
 	}
 
@@ -1579,6 +1593,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		die(_("cannot update HEAD ref"));
 	}
 
+	mention_abandoned_message = 0;
 	unlink(git_path("CHERRY_PICK_HEAD"));
 	unlink(git_path("REVERT_HEAD"));
 	unlink(git_path("MERGE_HEAD"));
-- 
1.7.10.5.40.g059818d

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 18:52       ` [PATCH 3/3] commit: give a hint when a commit message has been abandoned Jeff King
@ 2012-07-23 20:49         ` Junio C Hamano
  2012-07-23 20:52           ` Jeff King
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-07-23 20:49 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramana Kumar, git

Jeff King <peff@peff.net> writes:

> If we launch an editor for the user to create a commit
> message, they may put significant work into doing so.
> Typically we try to check common mistakes that could cause
> the commit to fail early, so that we die before the user
> goes to the trouble.
>
> We may still experience some errors afterwards, though; in
> this case, the user is given no hint that their commit
> message has been saved. Let's tell them where it is.

Liberal use of atexit() for something like this makes me cringe
somewhat.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I did not bother protecting this with advice.* config, as it is unlikely
> to come up regularly. If somebody cares, they are welcome to add it on
> top.
>
>  builtin/commit.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 20cef95..149e07d 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -116,6 +116,16 @@ static enum {
>  	STATUS_FORMAT_PORCELAIN
>  } status_format = STATUS_FORMAT_LONG;
>  
> +static int mention_abandoned_message;
> +static void maybe_mention_abandoned_message(void)
> +{
> +	if (!mention_abandoned_message)
> +		return;
> +	advise(_("Your commit message has been saved in '%s' and will be\n"
> +		 "overwritten by the next invocation of \"git commit\"."),
> +	       git_path(commit_editmsg));
> +}
> +
>  static int opt_parse_m(const struct option *opt, const char *arg, int unset)
>  {
>  	struct strbuf *buf = opt->value;
> @@ -848,6 +858,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			_("Please supply the message using either -m or -F option.\n"));
>  			exit(1);
>  		}
> +		atexit(maybe_mention_abandoned_message);
> +		mention_abandoned_message = 1;
>  	}
>  
>  	if (!no_verify &&
> @@ -1532,11 +1544,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	if (template_untouched(&sb) && !allow_empty_message) {
>  		rollback_index_files();
>  		fprintf(stderr, _("Aborting commit; you did not edit the message.\n"));
> +		mention_abandoned_message = 0;
>  		exit(1);
>  	}
>  	if (message_is_empty(&sb) && !allow_empty_message) {
>  		rollback_index_files();
>  		fprintf(stderr, _("Aborting commit due to empty commit message.\n"));
> +		mention_abandoned_message = 0;
>  		exit(1);
>  	}
>  
> @@ -1579,6 +1593,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		die(_("cannot update HEAD ref"));
>  	}
>  
> +	mention_abandoned_message = 0;
>  	unlink(git_path("CHERRY_PICK_HEAD"));
>  	unlink(git_path("REVERT_HEAD"));
>  	unlink(git_path("MERGE_HEAD"));

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] commit: check committer identity more strictly
  2012-07-23 18:50       ` [PATCH 2/3] commit: check committer identity more strictly Jeff King
@ 2012-07-23 20:51         ` Junio C Hamano
  2012-07-23 20:53           ` Jeff King
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-07-23 20:51 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramana Kumar, git

Jeff King <peff@peff.net> writes:

> Incidentally, this bug was masked prior to 060d4bb, as the
> initial loose call would taint the later strict call. So the
> commit would succeed (albeit with a bogus committer line in
> the commit object), and nobody noticed that our early check
> did not match the later one.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/commit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 95eeab1..20cef95 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -725,7 +725,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	strbuf_release(&sb);
>  
>  	/* This checks if committer ident is explicitly given */
> -	strbuf_addstr(&committer_ident, git_committer_info(0));
> +	strbuf_addstr(&committer_ident, git_committer_info(IDENT_STRICT));
>  	if (use_editor && include_status) {
>  		char *ai_tmp, *ci_tmp;
>  		if (whence != FROM_COMMIT)

Looks sensible.  Is this something we can detect in automated tests,
or is it too cumbersome to set up?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 20:49         ` Junio C Hamano
@ 2012-07-23 20:52           ` Jeff King
  2012-07-23 21:00             ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2012-07-23 20:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramana Kumar, git

On Mon, Jul 23, 2012 at 01:49:55PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If we launch an editor for the user to create a commit
> > message, they may put significant work into doing so.
> > Typically we try to check common mistakes that could cause
> > the commit to fail early, so that we die before the user
> > goes to the trouble.
> >
> > We may still experience some errors afterwards, though; in
> > this case, the user is given no hint that their commit
> > message has been saved. Let's tell them where it is.
> 
> Liberal use of atexit() for something like this makes me cringe
> somewhat.

I don't like it either, but there's not really a better way. The die()
that Ramana triggered in the initial report is deep inside the ident
code. The only other option would be to hook into die_routine, which I
think is even uglier.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] commit: check committer identity more strictly
  2012-07-23 20:51         ` Junio C Hamano
@ 2012-07-23 20:53           ` Jeff King
  0 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2012-07-23 20:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramana Kumar, git

On Mon, Jul 23, 2012 at 01:51:25PM -0700, Junio C Hamano wrote:

> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 95eeab1..20cef95 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -725,7 +725,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> >  	strbuf_release(&sb);
> >  
> >  	/* This checks if committer ident is explicitly given */
> > -	strbuf_addstr(&committer_ident, git_committer_info(0));
> > +	strbuf_addstr(&committer_ident, git_committer_info(IDENT_STRICT));
> >  	if (use_editor && include_status) {
> >  		char *ai_tmp, *ci_tmp;
> >  		if (whence != FROM_COMMIT)
> 
> Looks sensible.  Is this something we can detect in automated tests,
> or is it too cumbersome to set up?

Sorry, I meant to mention that in the cover letter. No, we can't test
this easily, because the code path in question is triggered by finding a
blank name in /etc/passwd. We'd have to override our getpwent lookup.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 20:52           ` Jeff King
@ 2012-07-23 21:00             ` Junio C Hamano
  2012-07-23 21:13               ` Jeff King
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-07-23 21:00 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramana Kumar, git

Jeff King <peff@peff.net> writes:

> On Mon, Jul 23, 2012 at 01:49:55PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > If we launch an editor for the user to create a commit
>> > message, they may put significant work into doing so.
>> > Typically we try to check common mistakes that could cause
>> > the commit to fail early, so that we die before the user
>> > goes to the trouble.
>> >
>> > We may still experience some errors afterwards, though; in
>> > this case, the user is given no hint that their commit
>> > message has been saved. Let's tell them where it is.
>> 
>> Liberal use of atexit() for something like this makes me cringe
>> somewhat.
>
> I don't like it either, but there's not really a better way. The die()
> that Ramana triggered in the initial report is deep inside the ident
> code. The only other option would be to hook into die_routine, which I
> think is even uglier.

Then I would rather not worry about it.  A documentation update is
probably a good first step, though.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 21:00             ` Junio C Hamano
@ 2012-07-23 21:13               ` Jeff King
  2012-07-23 21:35                 ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2012-07-23 21:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramana Kumar, git

On Mon, Jul 23, 2012 at 02:00:19PM -0700, Junio C Hamano wrote:

> >> Liberal use of atexit() for something like this makes me cringe
> >> somewhat.
> >
> > I don't like it either, but there's not really a better way. The die()
> > that Ramana triggered in the initial report is deep inside the ident
> > code. The only other option would be to hook into die_routine, which I
> > think is even uglier.
> 
> Then I would rather not worry about it.  A documentation update is
> probably a good first step, though.

I'm OK with dropping this one; the likely cause is ident problems, and
the previous patch already helped with that (the next likely is probably
commit hooks failing, but that is just a guess).

Here's a documentation patch.

-- >8 --
Subject: [PATCH] commit: document the temporary commit message file

We do not document COMMIT_EDITMSG at all, but users may want
to know about it for two reasons:

  1. They may want to tell their editor to configure itself
     for formatting a commit message.

  2. If a commit is aborted by an error, the user may want
     to recover the commit message they typed.

Let's put a note in git-commit(1).
---
 Documentation/git-commit.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f400835..87297dc 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -407,6 +407,15 @@ This command can run `commit-msg`, `prepare-commit-msg`, `pre-commit`,
 and `post-commit` hooks.  See linkgit:githooks[5] for more
 information.
 
+FILES
+-----
+
+`$GIT_DIR/COMMIT_EDITMSG`::
+	This file contains the commit message of a commit in progress.
+	If `git-commit` exits due to an error before creating a commit,
+	any commit message that has been provided by the user (e.g., in
+	an editor session) will be available in this file, but will be
+	overwritten by the next invocation of `git-commit`.
 
 SEE ALSO
 --------

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 21:13               ` Jeff King
@ 2012-07-23 21:35                 ` Junio C Hamano
  2012-07-23 21:43                   ` Jeff King
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2012-07-23 21:35 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramana Kumar, git

Jeff King <peff@peff.net> writes:

> Here's a documentation patch.
>
> -- >8 --
> Subject: [PATCH] commit: document the temporary commit message file
>
> We do not document COMMIT_EDITMSG at all, but users may want
> to know about it for two reasons:
>
>   1. They may want to tell their editor to configure itself
>      for formatting a commit message.
>
>   2. If a commit is aborted by an error, the user may want
>      to recover the commit message they typed.
>
> Let's put a note in git-commit(1).
> ---
>  Documentation/git-commit.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index f400835..87297dc 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -407,6 +407,15 @@ This command can run `commit-msg`, `prepare-commit-msg`, `pre-commit`,
>  and `post-commit` hooks.  See linkgit:githooks[5] for more
>  information.
>  
> +FILES
> +-----
> +
> +`$GIT_DIR/COMMIT_EDITMSG`::
> +	This file contains the commit message of a commit in progress.
> +	If `git-commit` exits due to an error before creating a commit,
> +	any commit message that has been provided by the user (e.g., in
> +	an editor session) will be available in this file, but will be
> +	overwritten by the next invocation of `git-commit`.
>  
>  SEE ALSO
>  --------

Very sensible, modulo s/git-commit/git commit/ and lack of S-o-b.

I also wondered if something like the following might be useful, but
it probably falls into the "water under the bridge" category.

 builtin/commit.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 149e07d..83bcee4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -768,6 +768,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				"with '#' will be kept; you may remove them"
 				" yourself if you want to.\n"
 				"An empty message aborts the commit.\n"));
+		status_printf(s, GIT_COLOR_NORMAL,
+			      _("The file '%s' keeps a copy of the log message\n"
+				"you edited, if you wish to inspect it later.\n"
+				"It will be wiped by the next invocation of 'git commit'.\n"),
+			      git_path("COMMIT_EDITMSG"));
 		if (only_include_assumed)
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					"%s", only_include_assumed);

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 21:35                 ` Junio C Hamano
@ 2012-07-23 21:43                   ` Jeff King
  2012-07-23 22:09                     ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2012-07-23 21:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ramana Kumar, git

On Mon, Jul 23, 2012 at 02:35:01PM -0700, Junio C Hamano wrote:

> > +FILES
> > +-----
> > +
> > +`$GIT_DIR/COMMIT_EDITMSG`::
> > +	This file contains the commit message of a commit in progress.
> > +	If `git-commit` exits due to an error before creating a commit,
> > +	any commit message that has been provided by the user (e.g., in
> > +	an editor session) will be available in this file, but will be
> > +	overwritten by the next invocation of `git-commit`.
> >  
> >  SEE ALSO
> >  --------
> 
> Very sensible, modulo s/git-commit/git commit/ and lack of S-o-b.

Yeah, I'm used to using the dashed form in documentation, but it's
probably reasonable to use the normal spaced form here. I assume you can
forge my S-o-b?

> I also wondered if something like the following might be useful, but
> it probably falls into the "water under the bridge" category.
> 
>  builtin/commit.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 149e07d..83bcee4 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -768,6 +768,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				"with '#' will be kept; you may remove them"
>  				" yourself if you want to.\n"
>  				"An empty message aborts the commit.\n"));
> +		status_printf(s, GIT_COLOR_NORMAL,
> +			      _("The file '%s' keeps a copy of the log message\n"
> +				"you edited, if you wish to inspect it later.\n"
> +				"It will be wiped by the next invocation of 'git commit'.\n"),
> +			      git_path("COMMIT_EDITMSG"));

That seems excessive, as it is inside the file itself. Unless your
editor is terrible, it already shows you that information.

-Peff

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] commit: give a hint when a commit message has been abandoned
  2012-07-23 21:43                   ` Jeff King
@ 2012-07-23 22:09                     ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2012-07-23 22:09 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramana Kumar, git

Jeff King <peff@peff.net> writes:

> On Mon, Jul 23, 2012 at 02:35:01PM -0700, Junio C Hamano wrote:
> ...
>> I also wondered if something like the following might be useful, but
>> it probably falls into the "water under the bridge" category.
>> 
>>  builtin/commit.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 149e07d..83bcee4 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -768,6 +768,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>>  				"with '#' will be kept; you may remove them"
>>  				" yourself if you want to.\n"
>>  				"An empty message aborts the commit.\n"));
>> +		status_printf(s, GIT_COLOR_NORMAL,
>> +			      _("The file '%s' keeps a copy of the log message\n"
>> +				"you edited, if you wish to inspect it later.\n"
>> +				"It will be wiped by the next invocation of 'git commit'.\n"),
>> +			      git_path("COMMIT_EDITMSG"));
>
> That seems excessive, as it is inside the file itself. Unless your
> editor is terrible, it already shows you that information.

The pathname was not the part that was interesting; "If you wish to
inspect it later" was.

But that is what makes it water under the bridge.  The message will
be gone by the time you really need it.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-07-23 22:09 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CAMej=25=xj61pc+k42dv3byuBqUJzW21Sz+BXwoufqnKwV5Bbg@mail.gmail.com>
2012-07-21 14:26 ` empty ident name trashes commit message Ramana Kumar
2012-07-23 17:27   ` Jeff King
2012-07-23 18:05     ` Ramana Kumar
2012-07-23 18:46     ` Jeff King
2012-07-23 18:48       ` [PATCH 1/3] advice: pass varargs to strbuf_vaddf, not strbuf_addf Jeff King
2012-07-23 18:50       ` [PATCH 2/3] commit: check committer identity more strictly Jeff King
2012-07-23 20:51         ` Junio C Hamano
2012-07-23 20:53           ` Jeff King
2012-07-23 18:52       ` [PATCH 3/3] commit: give a hint when a commit message has been abandoned Jeff King
2012-07-23 20:49         ` Junio C Hamano
2012-07-23 20:52           ` Jeff King
2012-07-23 21:00             ` Junio C Hamano
2012-07-23 21:13               ` Jeff King
2012-07-23 21:35                 ` Junio C Hamano
2012-07-23 21:43                   ` Jeff King
2012-07-23 22:09                     ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.