All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] git-commit doc: say -t requires editing commit message
@ 2012-03-29 17:57 Adam Monsen
  2012-03-29 18:09 ` Ivan Heffner
  0 siblings, 1 reply; 19+ messages in thread
From: Adam Monsen @ 2012-03-29 17:57 UTC (permalink / raw)
  To: git; +Cc: Adam Monsen

Make it clear that, when using a commit template, the message *must* be
changed or the commit will be aborted "due to empty commit message".

Signed-off-by: Adam Monsen <haircut@gmail.com>
---

I found it confusing that the commit template itself, even if
non-empty, must be edited. Hopefully this clears that up a bit.

 Documentation/git-commit.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 5cc84a1..44947ab 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -133,7 +133,7 @@ OPTIONS
 -t <file>::
 --template=<file>::
 	Use the contents of the given file as the initial version
-	of the commit message. The editor is invoked and you can
+	of the commit message. The editor is invoked and you must
 	make subsequent changes. If a message is specified using
 	the `-m` or `-F` options, this option has no effect. This
 	overrides the `commit.template` configuration variable.
-- 
1.7.5.4

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

* Re: [PATCH] git-commit doc: say -t requires editing commit message
  2012-03-29 17:57 [PATCH] git-commit doc: say -t requires editing commit message Adam Monsen
@ 2012-03-29 18:09 ` Ivan Heffner
  2012-03-29 23:04   ` [PATCH v2] git-commit.txt: clarify -t requires editing message Adam Monsen
  0 siblings, 1 reply; 19+ messages in thread
From: Ivan Heffner @ 2012-03-29 18:09 UTC (permalink / raw)
  To: Adam Monsen; +Cc: git

I'd suggest being much more verbose about what's going on and why.

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 5cc84a1..e842916 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -133,10 +133,12 @@ OPTIONS
 -t <file>::
 --template=<file>::
        Use the contents of the given file as the initial version
-       of the commit message. The editor is invoked and you can
-       make subsequent changes. If a message is specified using
-       the `-m` or `-F` options, this option has no effect. This
-       overrides the `commit.template` configuration variable.
+       of the commit message. The editor is invoked so you can
+       make changes. If a message is specified using the `-m` or `-F`
+       options, this option has no effect. This overrides the
+       `commit.template` configuration variable. If the message is
+       unchanged, the message is considered to be empty and the commit is
+       aborted

 -s::
 --signoff::
--
1.7.6.553.g917d7.dirty

On Thu, Mar 29, 2012 at 10:57 AM, Adam Monsen <haircut@gmail.com> wrote:
>
> Make it clear that, when using a commit template, the message *must* be
> changed or the commit will be aborted "due to empty commit message".
>
> Signed-off-by: Adam Monsen <haircut@gmail.com>
> ---
>
> I found it confusing that the commit template itself, even if
> non-empty, must be edited. Hopefully this clears that up a bit.
>
>  Documentation/git-commit.txt |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 5cc84a1..44947ab 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -133,7 +133,7 @@ OPTIONS
>  -t <file>::
>  --template=<file>::
>        Use the contents of the given file as the initial version
> -       of the commit message. The editor is invoked and you can
> +       of the commit message. The editor is invoked and you must
>        make subsequent changes. If a message is specified using
>        the `-m` or `-F` options, this option has no effect. This
>        overrides the `commit.template` configuration variable.
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-29 18:09 ` Ivan Heffner
@ 2012-03-29 23:04   ` Adam Monsen
  2012-03-30  2:05     ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Adam Monsen @ 2012-03-29 23:04 UTC (permalink / raw)
  To: git; +Cc: Ivan Heffner, Adam Monsen

Make it clear that, when using a commit template, the message *must* be
changed or the commit will be aborted "due to empty commit message".

Helped-by: Ivan Heffner <iheffner@gmail.com>
Signed-off-by: Adam Monsen <haircut@gmail.com>
---
Incorporate feedback from Ivan Heffner.

Anyone else agree/disagree with this patch? I would like to see the
manpage improved because I found the behavior of "git commit -t"
confusing as documented.

I wrapped the text at 77 characters because that was the longest
line in the file (according to wc -L).

I used ":set noet nosta ts=8 sw=8 tw=77" in Vim.

 Documentation/git-commit.txt |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 5cc84a1..c6df120 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -133,10 +133,12 @@ OPTIONS
 -t <file>::
 --template=<file>::
 	Use the contents of the given file as the initial version
-	of the commit message. The editor is invoked and you can
-	make subsequent changes. If a message is specified using
-	the `-m` or `-F` options, this option has no effect. This
-	overrides the `commit.template` configuration variable.
+	of the commit message. The editor is invoked so you can
+	make subsequent changes. If you make no changes, the message
+	is considered empty and the commit is aborted. If a message
+	is specified using the `-m` or `-F` options, this option has
+	no effect. This overrides the `commit.template`
+	configuration variable.
 
 -s::
 --signoff::
-- 
1.7.5.4

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

* Re: [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-29 23:04   ` [PATCH v2] git-commit.txt: clarify -t requires editing message Adam Monsen
@ 2012-03-30  2:05     ` Junio C Hamano
  2012-03-30  3:07       ` Adam Monsen
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30  2:05 UTC (permalink / raw)
  To: Adam Monsen; +Cc: git, Ivan Heffner

Adam Monsen <haircut@gmail.com> writes:

> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 5cc84a1..c6df120 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -133,10 +133,12 @@ OPTIONS
>  -t <file>::
>  --template=<file>::
>  	Use the contents of the given file as the initial version
> +	of the commit message. The editor is invoked so you can
> +	make subsequent changes. If you make no changes, the message
> +	is considered empty and the commit is aborted. If a message
> +	is specified using the `-m` or `-F` options, this option has
> +	no effect. This overrides the `commit.template`
> +	configuration variable.

First, think of template not as the "initial version" but as "a form that
needs to be filled", and imagine that you are explaining to a beginner how
to create a commit.

The word you would choose to use would be very different if you rephrase
the above after doing that mental exercise, and I suspect that it would
become much easier to read.

For example:

    - subsequent changes --> fill in the form
    - If ... considerede empty and --> If you did not fill the form

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

* Re: [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-30  2:05     ` Junio C Hamano
@ 2012-03-30  3:07       ` Adam Monsen
  2012-03-30  3:52         ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Adam Monsen @ 2012-03-30  3:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ivan Heffner

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

Junio C. Hamano wrote:
> First, think of template not as the "initial version" but as "a form
> that needs to be filled"

Ah, that clarifies for me what the authors had in mind when
implementing this feature.

I was hoping to use this feature to pre-populate the commit message
in my editor and have the option to edit it or leave it as-is. This
is what the word "template" means for me. For example: if I open a
"template" in LibreOffice, I can make changes or not, and
LibreOffice still lets me print or save at any point.

Now that I know it's not that at all, but rather something more like
a mandatory fillable form, I'll find another way to achieve my need.
Since git really wants me to enter *something* for a commit message,
I understand why git "templates" work differently than LibreOffice
"templates".

It's getting a little wordy, but here's an attempt to work in the
"form" concept:

~~~
Use the contents of the given file as the initial version of the
commit message. Think of this initial version as a mandatory
fillable form. The editor is invoked so you can fill in the form. If
you do not fill in the form (if you make no changes), the message is
considered empty and the commit is aborted. If a message is
specified using the `-m` or `-F` options, this option has no effect.
This overrides the `commit.template` configuration variable.
~~~

Thoughts?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

* Re: [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-30  3:07       ` Adam Monsen
@ 2012-03-30  3:52         ` Junio C Hamano
  2012-03-30  4:53           ` Adam Monsen
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30  3:52 UTC (permalink / raw)
  To: Adam Monsen; +Cc: git, Ivan Heffner

Adam Monsen <haircut@gmail.com> writes:

> ~~~
> Use the contents of the given file as the initial version of the
> commit message. Think of this initial version as a mandatory
> fillable form. The editor is invoked so you can fill in the form. If
> you do not fill in the form (if you make no changes), the message is
> considered empty and the commit is aborted. If a message is
> specified using the `-m` or `-F` options, this option has no effect.
> This overrides the `commit.template` configuration variable.
> ~~~
>
> Thoughts?

You still say "the message is considered empty and" but I think it
probably reads better without it.  Strictly speaking, it is not a
"mandatory fillable form", but whatever text you put in the template is
advisory to the users.  For example, if your project wants its contributor
to always refer to a bug id in its issue tracker, it may want to give a
customized "template", instead of the plain "template" we give to the
users that begins with:

	~~~~~~~~

        # Please enter the commit message for your changes. Lines starting
        # with '#' will be ignored, and an empty message aborts the commit.
	~~~~~~~~

to guide them what to write in the log and how to explain your change,
e.g. something like:

	~~~~~~~~
	<<one line summary your change here>>

	# explain the problem your change tries to solve in the first
        # paragraph

	# describe the approach your solution takes to solve it in the
        # second and subsequent paragraphs

	# Please write issue tracker ID at the end, if available
	Frotz-Bug-Id: XXXXXX

        # Always sign-off your commit
        Signed-off-by: XXXXXX
	~~~~~~~~

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

* Re: [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-30  3:52         ` Junio C Hamano
@ 2012-03-30  4:53           ` Adam Monsen
  2012-03-30  5:08             ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Adam Monsen @ 2012-03-30  4:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ivan Heffner

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Junio C Hamano wrote:
> You still say "the message is considered empty and" but I think it
>  probably reads better without it.

Do you like the patch without those words?

> Strictly speaking, it is not a "mandatory fillable form", but 
> whatever text you put in the template is advisory to the users.

Ok, right on. I understand the template feature now since you've
patiently explained it to me (thank you!). I still want to plainly
convey that even if my template is the following:

	~~~~
	zip module: continue to expand transformer
	
	This WIP will eventually provide expanded
	transformer functionality.
	~~~~

I *cannot* just save and quit my editor (unless I supply
- --allow-empty-message). That's the behavior I find confusing: git
telling me a non-empty commit message is an empty commit message. If I
save that text above (zip module... etc) in FILE and do `git commit -t
FILE`, save and quit my editor, git says "Aborting commit due to empty
commit message." Lies! A more precise message would be "Aborting
commit due to unmodified commit message template."

Based on the current documentation I misunderstood that -t could be
used to review a boilerplate commit message and save it verbatim.

...AHA! I just figured out a way to do exactly that:

	git commit --edit --file=FILE

aka

	git commit -eF FILE

Yay! No idea how I missed that before.

Anyway, it still seems like the documentation for "git commit -t"
could use improvement. I actually wouldn't mind seeing a short example
template like the one you provided, maybe in the EXAMPLES or
DISCUSSION section.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQEcBAEBAgAGBQJPdTw/AAoJEJtdmT+DbynAte4IALRx50yW5bEfzXskvSDewPuL
SgaU4CqUHRm8sWXHeFbd4I2rG4dEJuqYqzKKbfay3EMwEbIkThiwoC2pJ9xJoFpe
8O95GVp3ikYvsY3mn87ebiwA9FBhnTy1Fz+MREfuzETpJbdtJSHhbRXMxfJ9ZabU
FOPE/qeZDvQJA9b9QFY3QS/BcxsGHXhW9xCULZlAprDggMcchhHDEbqJCh/1wObw
cQvoONiqZSkXA17K3gxfs7NgafUVFIg3+N9vcq90eZXbT/s1MM+1zxj5ezTh9jbV
sOkzepfE5+NBK3PnewMDxDxhF0LD5lzHCwnfkTl1Om3okSE0nxVyRZKabuzc99s=
=NUN8
-----END PGP SIGNATURE-----

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

* Re: [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-30  4:53           ` Adam Monsen
@ 2012-03-30  5:08             ` Junio C Hamano
  2012-03-30  5:43               ` Adam Monsen
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30  5:08 UTC (permalink / raw)
  To: Adam Monsen; +Cc: git, Ivan Heffner

Adam Monsen <haircut@gmail.com> writes:

> I *cannot* just save and quit my editor (unless I supply
> --allow-empty-message). That's the behavior I find confusing...

The root cause of the confusion is that the entire "--template" code piggy
backs on the non-templated case, where we would want to error out if the
user leaves the editor without explaining the commit. And an appropriate
diagnosis message for the "normal" case is "you gave me an empty message",
but "--template" code did not bother updating that to suit what it tries
to do better, e.g. "you did not edit the template I gave you".

The check to see if the message the user left matches that came from the
template was tacked into a wrong function message_is_empty(); it should be
made into its own helper function and called from the same caller where
it calls message_is_empty() only when template_file is not NULL.  Also its
honoring "--allow-empty" needs to be reconsidered.

In other words, I think that is something that needs fixing the broken
code to behave less confusingly, not documenting its wrong behaviour.

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

* Re: [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-30  5:08             ` Junio C Hamano
@ 2012-03-30  5:43               ` Adam Monsen
  2012-03-30 18:17                 ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Adam Monsen @ 2012-03-30  5:43 UTC (permalink / raw)
  To: Junio C Hamano, Ivan Heffner; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 277 bytes --]

Junio C Hamano wrote:
> I think that is something that needs fixing the broken code to behave
> less confusingly, not documenting its wrong behaviour.

Excellent! I concur.

I wish I wanted to do this enough to make time to work on it. Ivan, how
are your C chops? :)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

* Re: [PATCH v2] git-commit.txt: clarify -t requires editing message
  2012-03-30  5:43               ` Adam Monsen
@ 2012-03-30 18:17                 ` Junio C Hamano
  2012-03-30 19:45                   ` [PATCH 0/3] "commit --template" fixes Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30 18:17 UTC (permalink / raw)
  To: Adam Monsen; +Cc: Ivan Heffner, git

Adam Monsen <haircut@gmail.com> writes:

> Junio C Hamano wrote:
>> I think that is something that needs fixing the broken code to behave
>> less confusingly, not documenting its wrong behaviour.
>
> Excellent! I concur.
>
> I wish I wanted to do this enough to make time to work on it. Ivan, how
> are your C chops? :)

Don't worry.  While looking around the vicinity of the codepath, I noticed
a few more bugs there, so I'll post something today.

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

* [PATCH 0/3] "commit --template" fixes
  2012-03-30 18:17                 ` Junio C Hamano
@ 2012-03-30 19:45                   ` Junio C Hamano
  2012-03-30 19:45                     ` [PATCH 1/3] t7501: test the right kind of breakage Junio C Hamano
                                       ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30 19:45 UTC (permalink / raw)
  To: git; +Cc: Adam Monsen

When the user exited editor without editing the commit log template given
by "git commit -t <template>", the commit was aborted (correct) with an
error message that said "due to empty commit message" (incorrect).  The
goal of this series is to fix this, which is the third patch.

While looking at this, I found another bug that the contents of the
template file is still used for error checking even when it is ignored
when the editor is populated for the user to edit.  The second patch
addresses this.

Junio C Hamano (3):
  t7501: test the right kind of breakage
  commit: do not trigger bogus "has templated message edited" check
  commit: rephrase the error when user did not touch templated log message

 builtin/commit.c  |   62 +++++++++++++++++++++++++++++++++++++----------------
 t/t7501-commit.sh |   14 ++++++++++++
 2 files changed, 57 insertions(+), 19 deletions(-)

-- 
1.7.10.rc3.55.g06e99

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

* [PATCH 1/3] t7501: test the right kind of breakage
  2012-03-30 19:45                   ` [PATCH 0/3] "commit --template" fixes Junio C Hamano
@ 2012-03-30 19:45                     ` Junio C Hamano
  2012-03-30 19:45                     ` [PATCH 2/3] commit: do not trigger bogus "has templated message edited" check Junio C Hamano
                                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30 19:45 UTC (permalink / raw)
  To: git; +Cc: Adam Monsen

These tests try to run "git commit" with various "forbidden" combinations
of options and expect the command to fail, but they do so without having
any change added to the index.  We wouldn't be able to catch breakages
that would allow these combinations by mistake with them because the
command will fail with "nothing to commit" anyway.

Make sure we have something added to the index before running the command.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7501-commit.sh |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 8bb3833..45446b1 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -30,10 +30,12 @@ test_expect_success 'setup: initial commit' '
 '
 
 test_expect_success '-m and -F do not mix' '
+	git checkout HEAD file && echo >>file && git add file &&
 	test_must_fail git commit -m foo -m bar -F file
 '
 
 test_expect_success '-m and -C do not mix' '
+	git checkout HEAD file && echo >>file && git add file &&
 	test_must_fail git commit -C HEAD -m illegal
 '
 
-- 
1.7.10.rc3.55.g06e99

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

* [PATCH 2/3] commit: do not trigger bogus "has templated message edited" check
  2012-03-30 19:45                   ` [PATCH 0/3] "commit --template" fixes Junio C Hamano
  2012-03-30 19:45                     ` [PATCH 1/3] t7501: test the right kind of breakage Junio C Hamano
@ 2012-03-30 19:45                     ` Junio C Hamano
  2012-03-30 19:45                     ` [PATCH 3/3] commit: rephrase the error when user did not touch templated log message Junio C Hamano
  2012-03-31 19:28                     ` [PATCH 0/3] "commit --template" fixes Adam Monsen
  3 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30 19:45 UTC (permalink / raw)
  To: git; +Cc: Adam Monsen

When "-t template" and "-F msg" options are both given (or worse yet,
there is "commit.template" configuration but a message is given in some
other way), the documentation says that template is ignored.  However,
the "has the user edited the message?" check still used the contents of
the template file as the basis of the emptyness check.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c  |    2 ++
 t/t7501-commit.sh |    6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index eba1377..7141766 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1049,6 +1049,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die(_("Only one of -c/-C/-F/--fixup can be used."));
 	if (message.len && f > 0)
 		die((_("Option -m cannot be combined with -c/-C/-F/--fixup.")));
+	if (f || message.len)
+		template_file = NULL;
 	if (edit_message)
 		use_message = edit_message;
 	if (amend && !use_message && !fixup_message)
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 45446b1..e59cc4e 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -81,7 +81,13 @@ test_expect_success 'empty commit message' '
 	test_must_fail git commit -F msg -a
 '
 
+test_expect_success 'template "emptyness" check does not kick in with -F' '
+	git checkout HEAD file && echo >>file && git add file &&
+	git commit -t file -F file
+'
+
 test_expect_success 'setup: commit message from file' '
+	git checkout HEAD file && echo >>file && git add file &&
 	echo this is the commit message, coming from a file >msg &&
 	git commit -F msg -a
 '
-- 
1.7.10.rc3.55.g06e99

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

* [PATCH 3/3] commit: rephrase the error when user did not touch templated log message
  2012-03-30 19:45                   ` [PATCH 0/3] "commit --template" fixes Junio C Hamano
  2012-03-30 19:45                     ` [PATCH 1/3] t7501: test the right kind of breakage Junio C Hamano
  2012-03-30 19:45                     ` [PATCH 2/3] commit: do not trigger bogus "has templated message edited" check Junio C Hamano
@ 2012-03-30 19:45                     ` Junio C Hamano
  2012-03-31 19:28                     ` [PATCH 0/3] "commit --template" fixes Adam Monsen
  3 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2012-03-30 19:45 UTC (permalink / raw)
  To: git; +Cc: Adam Monsen

When the user exited editor without editing the commit log template given
by "git commit -t <template>", the commit was aborted (correct) with an
error message that said "due to empty commit message" (incorrect).

This was because the original template support was done by piggybacking on
the check to detect an empty log message.  Split the codepaths into two
independent checks to clarify the error.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c  |   60 ++++++++++++++++++++++++++++++++++++-----------------
 t/t7501-commit.sh |    6 ++++++
 2 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 7141766..847d363 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -899,27 +899,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	return 1;
 }
 
-/*
- * Find out if the message in the strbuf contains only whitespace and
- * Signed-off-by lines.
- */
-static int message_is_empty(struct strbuf *sb)
+static int rest_is_empty(struct strbuf *sb, int start)
 {
-	struct strbuf tmpl = STRBUF_INIT;
+	int i, eol;
 	const char *nl;
-	int eol, i, start = 0;
-
-	if (cleanup_mode == CLEANUP_NONE && sb->len)
-		return 0;
-
-	/* See if the template is just a prefix of the message. */
-	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
-		stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);
-		if (start + tmpl.len <= sb->len &&
-		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
-			start += tmpl.len;
-	}
-	strbuf_release(&tmpl);
 
 	/* Check if the rest is just whitespace and Signed-of-by's. */
 	for (i = start; i < sb->len; i++) {
@@ -942,6 +925,40 @@ static int message_is_empty(struct strbuf *sb)
 	return 1;
 }
 
+/*
+ * Find out if the message in the strbuf contains only whitespace and
+ * Signed-off-by lines.
+ */
+static int message_is_empty(struct strbuf *sb)
+{
+	if (cleanup_mode == CLEANUP_NONE && sb->len)
+		return 0;
+	return rest_is_empty(sb, 0);
+}
+
+/*
+ * See if the user edited the message in the editor or left what
+ * was in the template intact
+ */
+static int template_untouched(struct strbuf *sb)
+{
+	struct strbuf tmpl = STRBUF_INIT;
+	char *start;
+
+	if (cleanup_mode == CLEANUP_NONE && sb->len)
+		return 0;
+
+	if (!template_file || strbuf_read_file(&tmpl, template_file, 0) <= 0)
+		return 0;
+
+	stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);
+	start = (char *)skip_prefix(sb->buf, tmpl.buf);
+	if (!start)
+		start = sb->buf;
+	strbuf_release(&tmpl);
+	return rest_is_empty(sb, start - sb->buf);
+}
+
 static const char *find_author_by_nickname(const char *name)
 {
 	struct rev_info revs;
@@ -1490,6 +1507,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
+	if (template_untouched(&sb) && !allow_empty_message) {
+		rollback_index_files();
+		fprintf(stderr, _("Aborting commit; you did not edit the message.\n"));
+		exit(1);
+	}
 	if (message_is_empty(&sb) && !allow_empty_message) {
 		rollback_index_files();
 		fprintf(stderr, _("Aborting commit due to empty commit message.\n"));
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index e59cc4e..b20ca0e 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -86,6 +86,12 @@ test_expect_success 'template "emptyness" check does not kick in with -F' '
 	git commit -t file -F file
 '
 
+test_expect_success 'template "emptyness" check' '
+	git checkout HEAD file && echo >>file && git add file &&
+	test_must_fail git commit -t file 2>err &&
+	test_i18ngrep "did not edit" err
+'
+
 test_expect_success 'setup: commit message from file' '
 	git checkout HEAD file && echo >>file && git add file &&
 	echo this is the commit message, coming from a file >msg &&
-- 
1.7.10.rc3.55.g06e99

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

* Re: [PATCH 0/3] "commit --template" fixes
  2012-03-30 19:45                   ` [PATCH 0/3] "commit --template" fixes Junio C Hamano
                                       ` (2 preceding siblings ...)
  2012-03-30 19:45                     ` [PATCH 3/3] commit: rephrase the error when user did not touch templated log message Junio C Hamano
@ 2012-03-31 19:28                     ` Adam Monsen
  2012-04-01 22:28                       ` Junio C Hamano
  3 siblings, 1 reply; 19+ messages in thread
From: Adam Monsen @ 2012-03-31 19:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ivan Heffner

[-- Attachment #1: Type: text/plain, Size: 2950 bytes --]

On 03/30/2012 12:45 PM, Junio C Hamano wrote:
> When the user exited editor without editing the commit log template 
> given by "git commit -t <template>", the commit was aborted (correct)
> with an error message that said "due to empty commit message"
> (incorrect).  The goal of this series is to fix this, which is the
> third patch.

This is awesome. thanks!

I really like the new error message specific to the situation when
the user does not edit the template (as we discussed).

Your patches apply cleanly to maint b8939b2b3abaa.

I tested the patches and they work as expected. When I use
`git commit --template FILE` but do not edit the message in my editor,
I get

	Aborting commit; you did not edit the message.

Nice.

Only thing I'd add is a change to the git-commit(1) manpage.

* I prefer pragmatically explaining what will happen when the user
  uses --template but does not edit the message because it is more
  direct and terse (than "filling in a form").
* The below applies cleanly to maint as of today.
* I don't know the kosher procedure to add this commit to your patch
  series for further review, so hopefully this works.
* I'm not sure if the "Helped-by:" lines are kosher, I'm happy to
  remove them if not.

From 91a62baa1fe89032e7a3598e5d39241f3eb8f84b Mon Sep 17 00:00:00 2001
From: Adam Monsen <haircut@gmail.com>
Date: Sat, 31 Mar 2012 12:09:29 -0700
Subject: [PATCH] git-commit.txt: clarify -t requires editing message

Make it clear that, when using commit --template, the message *must* be
changed or the commit will be aborted.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Ivan Heffner <iheffner@gmail.com>
Signed-off-by: Adam Monsen <haircut@gmail.com>
---
I wrapped the text at 77 characters because that was the longest
line in the file (according to wc -L).

I used ":set noet nosta ts=8 sw=8 tw=77" in Vim.

 Documentation/git-commit.txt |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 5cc84a1..f584a62 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -132,11 +132,11 @@ OPTIONS
 
 -t <file>::
 --template=<file>::
-	Use the contents of the given file as the initial version
-	of the commit message. The editor is invoked and you can
-	make subsequent changes. If a message is specified using
-	the `-m` or `-F` options, this option has no effect. This
-	overrides the `commit.template` configuration variable.
+	Use the contents of the given file as the initial version of the
+	commit message. The editor is invoked so you can make subsequent
+	changes. If you make no changes, the commit is aborted. If a message
+	is specified using the `-m` or `-F` options, this option has no
+	effect. This overrides the `commit.template` configuration variable.
 
 -s::
 --signoff::
-- 
1.7.5.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

* Re: [PATCH 0/3] "commit --template" fixes
  2012-03-31 19:28                     ` [PATCH 0/3] "commit --template" fixes Adam Monsen
@ 2012-04-01 22:28                       ` Junio C Hamano
  2012-04-03 17:11                         ` Adam Monsen
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-04-01 22:28 UTC (permalink / raw)
  To: Adam Monsen; +Cc: git, Ivan Heffner

Adam Monsen <haircut@gmail.com> writes:

> ...
> * I don't know the kosher procedure to add this commit to your patch
>   series for further review, so hopefully this works.
> * I'm not sure if the "Helped-by:" lines are kosher, I'm happy to
>   remove them if not.

One established way to do this is to have a discussion like the above
(mostly elided), followed by a "scissors" line "-- >8 --", and then the
output from format-patch with most headers except for "Subject: " removed
(as From: and Date: will be taken from your e-mailed message anyway, and
the first "From <commit object name> <magic constant date>" is a signal to
allow automated tools to tell if it is a format-patch output or a random
mbox file, and is not appropriate if you are sending it over e-mail).

>
> From 91a62baa1fe89032e7a3598e5d39241f3eb8f84b Mon Sep 17 00:00:00 2001
> From: Adam Monsen <haircut@gmail.com>
> Date: Sat, 31 Mar 2012 12:09:29 -0700
> Subject: [PATCH] git-commit.txt: clarify -t requires editing message
>
> Make it clear that, when using commit --template, the message *must* be
> changed or the commit will be aborted.
>
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Ivan Heffner <iheffner@gmail.com>
> Signed-off-by: Adam Monsen <haircut@gmail.com>
> ---
> I wrapped the text at 77 characters because that was the longest
> line in the file (according to wc -L).
>
> I used ":set noet nosta ts=8 sw=8 tw=77" in Vim.

When rewording or clarifying only a handful of words in the documentation,
it is often better to avoid reflowing lines in the same patch.  It makes
it harder to see what you really changed, and what is merely reflowed.

I'll queue it as-is, though.  Thanks.

This is a tangent, but we might want to rephrase the first sentence
without using the word "version"; every time I read this paragraph, the
"initial version" makes me go "Huh?" because the word sounds as if it is
talking about commits in the context of SCM, which is not the case here.

I know that the description wanted to avoid use of the word "template" to
explain what the template is, but still...

>  Documentation/git-commit.txt |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 5cc84a1..f584a62 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -132,11 +132,11 @@ OPTIONS
>  
>  -t <file>::
>  --template=<file>::
> -	Use the contents of the given file as the initial version
> -	of the commit message. The editor is invoked and you can
> -	make subsequent changes. If a message is specified using
> -	the `-m` or `-F` options, this option has no effect. This
> -	overrides the `commit.template` configuration variable.
> +	Use the contents of the given file as the initial version of the
> +	commit message. The editor is invoked so you can make subsequent
> +	changes. If you make no changes, the commit is aborted. If a message
> +	is specified using the `-m` or `-F` options, this option has no
> +	effect. This overrides the `commit.template` configuration variable.
>  
>  -s::
>  --signoff::

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

* Re: [PATCH 0/3] "commit --template" fixes
  2012-04-01 22:28                       ` Junio C Hamano
@ 2012-04-03 17:11                         ` Adam Monsen
  2012-04-03 21:55                           ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Adam Monsen @ 2012-04-03 17:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ivan Heffner

On 04/01/2012 03:28 PM, Junio C Hamano wrote:
> One established way to do this is to have a discussion like the above
> (mostly elided), followed by a "scissors" line...

I can do that. Thanks!

> When rewording or clarifying only a handful of words in the documentation,
> it is often better to avoid reflowing lines in the same patch.

I thought of that, but it made the right margin jagged. :)
My new suggestion (below) isolates the changes a bit better.

> This is a tangent, but we might want to rephrase the first sentence
> without using the word "version"; every time I read this paragraph, the
> "initial version" makes me go "Huh?" because the word sounds as if it is
> talking about commits in the context of SCM, which is not the case here.

Yeah, that bugs me too.

How about this? I'm a little bummed it doesn't include why
commit --template exists at all, but it reads well: terse and to the
point like (IMHO) a manpage should.

-- >8 --
Subject: [PATCH v4] git-commit.txt: clarify -t requires editing message

Make it clear that, when using commit --template, the message *must* be
changed or the commit will be aborted.

Also, remove the words "initial version" to avoid confusion. Commit
messages are not versioned independently of commits.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Ivan Heffner <iheffner@gmail.com>
Signed-off-by: Adam Monsen <haircut@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Replaces b0ad5e27803cd of jc/commit-unedited-template. I'm assuming that's ok
since the branch isn't merged into maint or master.

 Documentation/git-commit.txt |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 5cc84a1..bd82431 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -132,9 +132,9 @@ OPTIONS
 
 -t <file>::
 --template=<file>::
-	Use the contents of the given file as the initial version
-	of the commit message. The editor is invoked and you can
-	make subsequent changes. If a message is specified using
+	Use the contents of the given file as the commit message. The
+	editor is invoked so you can make subsequent changes. If you make no
+	changes, the commit is aborted. If a message is specified using
 	the `-m` or `-F` options, this option has no effect. This
 	overrides the `commit.template` configuration variable.
 
-- 
1.7.5.4

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

* Re: [PATCH 0/3] "commit --template" fixes
  2012-04-03 17:11                         ` Adam Monsen
@ 2012-04-03 21:55                           ` Junio C Hamano
  2012-04-05 14:29                             ` Adam Monsen
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2012-04-03 21:55 UTC (permalink / raw)
  To: Adam Monsen; +Cc: Junio C Hamano, git, Ivan Heffner

Adam Monsen <haircut@gmail.com> writes:

> How about this? I'm a little bummed it doesn't include why commit
> --template exists at all, but it reads well: terse and to the point like
> (IMHO) a manpage should.

Perhaps we should explain why somebody might want to use --template
instead of -F; personally, I do not think --template command line option
would not make much sense unless it is used as a part of a script that
enforces external constraints in a larger workflow, even though such a
project could instead require the participants to set commit.template to
one supplied by the project.  As an enforcement mechanism, use of such
stricter "commit wrapper" and commit.template configuration cannot be
mechanical and absolute either way, as Git is distributed and whatever
happens in the participant's repository is purely up to the participant.

>  -t <file>::
>  --template=<file>::
> +	Use the contents of the given file as the commit message. The
> +	editor is invoked so you can make subsequent changes. If you make no
> +	changes, the commit is aborted. If a message is specified using
>  	the `-m` or `-F` options, this option has no effect. This
>  	overrides the `commit.template` configuration variable.

When editing the commit message, start the editor with the contents in the
given file.  The `commit.template` configuration variable is often used to
give this option implicitly to the command.  This mechanism can be used by
projects that want to guide participants with some hints on what to write
in the message in what order.  If the user exits the editor without editing
the message, the commit is aborted.  This has no effect when a message is
given by other means, e.g. with the `-m` or `-F` options.

Hmm?

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

* Re: [PATCH 0/3] "commit --template" fixes
  2012-04-03 21:55                           ` Junio C Hamano
@ 2012-04-05 14:29                             ` Adam Monsen
  0 siblings, 0 replies; 19+ messages in thread
From: Adam Monsen @ 2012-04-05 14:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Ivan Heffner

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

On 04/03/2012 02:55 PM, Junio C Hamano wrote:
> When editing the commit message, start the editor with the contents in the
> given file.  The `commit.template` configuration variable is often used to
> give this option implicitly to the command.  This mechanism can be used by
> projects that want to guide participants with some hints on what to write
> in the message in what order.  If the user exits the editor without editing
> the message, the commit is aborted.  This has no effect when a message is
> given by other means, e.g. with the `-m` or `-F` options.

I like it!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

end of thread, other threads:[~2012-04-05 14:30 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-29 17:57 [PATCH] git-commit doc: say -t requires editing commit message Adam Monsen
2012-03-29 18:09 ` Ivan Heffner
2012-03-29 23:04   ` [PATCH v2] git-commit.txt: clarify -t requires editing message Adam Monsen
2012-03-30  2:05     ` Junio C Hamano
2012-03-30  3:07       ` Adam Monsen
2012-03-30  3:52         ` Junio C Hamano
2012-03-30  4:53           ` Adam Monsen
2012-03-30  5:08             ` Junio C Hamano
2012-03-30  5:43               ` Adam Monsen
2012-03-30 18:17                 ` Junio C Hamano
2012-03-30 19:45                   ` [PATCH 0/3] "commit --template" fixes Junio C Hamano
2012-03-30 19:45                     ` [PATCH 1/3] t7501: test the right kind of breakage Junio C Hamano
2012-03-30 19:45                     ` [PATCH 2/3] commit: do not trigger bogus "has templated message edited" check Junio C Hamano
2012-03-30 19:45                     ` [PATCH 3/3] commit: rephrase the error when user did not touch templated log message Junio C Hamano
2012-03-31 19:28                     ` [PATCH 0/3] "commit --template" fixes Adam Monsen
2012-04-01 22:28                       ` Junio C Hamano
2012-04-03 17:11                         ` Adam Monsen
2012-04-03 21:55                           ` Junio C Hamano
2012-04-05 14:29                             ` Adam Monsen

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.