Re: [PATCH v3] Advertise the ability to abort a commit

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

Re: [PATCH v3] Advertise the ability to abort a commit

Jeff King
On Wed, Jul 30, 2008 at 07:53:11PM +0200, Anders Melchiorsen wrote:

> An empty commit message is now treated as a normal situation, not an error.

As others have commented, I think the right way to say this is probably
"it is not reported to the user as an error, but still exits with a
non-zero exit status".

And I think it looks better.

But:

>   "# Please enter the commit message for your changes.\n"
> + "# To abort the commit, use an empty commit message.\n"
>   "# (Comment lines starting with '#' will ");

I still prefer a shortened version of these three lines, as I mentioned
earlier.

-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 v3] Advertise the ability to abort a commit

Junio C Hamano
Jeff King <[hidden email]> writes:

> On Wed, Jul 30, 2008 at 07:53:11PM +0200, Anders Melchiorsen wrote:
>
>> An empty commit message is now treated as a normal situation, not an error.
>
> As others have commented, I think the right way to say this is probably
> "it is not reported to the user as an error, but still exits with a
> non-zero exit status".
>
> And I think it looks better.
>
> But:
>
>>   "# Please enter the commit message for your changes.\n"
>> + "# To abort the commit, use an empty commit message.\n"
>>   "# (Comment lines starting with '#' will ");
>
> I still prefer a shortened version of these three lines, as I mentioned
> earlier.

I tend to agree; please make it so ;-)
--
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
|

Advertise the ability to abort a commit

Jeff King
From: Anders Melchiorsen <[hidden email]>

We explicitly let the user know that an empty commit message
will abort the commit. At the same time, we take the
opportunity to reword the template text a bit to keep it
more compact.

This patch also makes the "fatal: empty commit message?"
warning a bit less scary, since this is now a "feature"
instead of an error. However, we retain the non-zero exit
status to indicate to callers that nothing was committed.

[jk: I compacted the text and expanded the commit message
from Anders' original patch]

Signed-off-by: Anders Melchiorsen <[hidden email]>
Signed-off-by: Jeff King <[hidden email]>
---
 builtin-commit.c  |   18 ++++++++++++------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 9a11ca0..b783e6e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -554,13 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 
  fprintf(fp,
  "\n"
- "# Please enter the commit message for your changes.\n"
- "# (Comment lines starting with '#' will ");
+ "# Please enter the commit message for your changes.");
  if (cleanup_mode == CLEANUP_ALL)
- fprintf(fp, "not be included)\n");
+ fprintf(fp,
+ " Lines starting\n"
+ "# with '#' will be ignored, and an empty"
+ " message aborts the commit.\n");
  else /* CLEANUP_SPACE, that is. */
- fprintf(fp, "be kept.\n"
- "# You can remove them yourself if you want to)\n");
+ fprintf(fp,
+ " Lines starting\n"
+ "# with '#' will be kept; you may remove them"
+ " yourself if you want to.\n"
+ "# An empty message aborts the commit.\n");
  if (only_include_assumed)
  fprintf(fp, "# %s\n", only_include_assumed);
 
@@ -1003,7 +1008,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
  stripspace(&sb, cleanup_mode == CLEANUP_ALL);
  if (sb.len < header_len || message_is_empty(&sb, header_len)) {
  rollback_index_files();
- die("no commit message?  aborting commit.");
+ fprintf(stderr, "Aborting commit due to empty commit message.\n");
+ exit(1);
  }
  strbuf_addch(&sb, '\0');
  if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 4f2682e..3eb9fae 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -141,8 +141,8 @@ test_expect_success 'cleanup commit messages (strip,-F)' '
 
 echo "sample
 
-# Please enter the commit message for your changes.
-# (Comment lines starting with '#' will not be included)" >expect
+# Please enter the commit message for your changes. Lines starting
+# with '#' will be ignored, and an empty message aborts the commit." >expect
 
 test_expect_success 'cleanup commit messages (strip,-F,-e)' '
 
--
1.6.0.rc1.168.g8c00d.dirty
--
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 v3] Advertise the ability to abort a commit

Anders Melchiorsen
In reply to this post by Jeff King
Jeff King <[hidden email]> writes:

> I still prefer a shortened version of these three lines, as I
> mentioned earlier.

Yeah, and I obviously didn't :-). I think the line wrapped, run-on
sentence makes it look more busy, even if it is shorter. Here is a
final compromise proposal:

# Enter a commit message for your changes. Use an empty one to abort.
# (Comment lines starting with '#' will not be included)


As this is mostly a matter of personal opinion, I will stop here.


Cheers,
Anders.
--
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 v3] Advertise the ability to abort a commit

Jeff King
On Thu, Jul 31, 2008 at 09:28:45AM +0200, Anders Melchiorsen wrote:

> > I still prefer a shortened version of these three lines, as I
> > mentioned earlier.
>
> Yeah, and I obviously didn't :-). I think the line wrapped, run-on
> sentence makes it look more busy, even if it is shorter. Here is a
> final compromise proposal:

OK, I wasn't sure if I was being disagreed with or overlooked. ;)

> # Enter a commit message for your changes. Use an empty one to abort.
> # (Comment lines starting with '#' will not be included)

I don't like that as well as mine, but we are well into the realm of
personal preference. I am fine with whatever the list (or Junio)
decides.

-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 v3] Advertise the ability to abort a commit

Jeff King
In reply to this post by Junio C Hamano
On Wed, Jul 30, 2008 at 10:58:13PM -0700, Junio C Hamano wrote:

> >>   "# Please enter the commit message for your changes.\n"
> >> + "# To abort the commit, use an empty commit message.\n"
> >>   "# (Comment lines starting with '#' will ");
> >
> > I still prefer a shortened version of these three lines, as I mentioned
> > earlier.
>
> I tend to agree; please make it so ;-)

Hmm, I didn't realize you had already applied the original patch. Here
is my previous patch, rebased on top of the current master.

I like this wording, but there is perhaps some disagreement. I will let
you apply, tweak, or ignore as you desire. :)

Note that this still has the error message change that Anders put in a
later patch, but is not in master. Should that be a separate patch (I
really didn't anticipate this much discussion for such a simple change,
but I think there is a rule of thumb about patch size and bike
sheds...)?

-- >8 --
Compact commit template message

We recently let the user know explicitly that an empty
commit message will abort the commit. However, this adds yet
another line to the template; let's rephrase and re-wrap so
that this fits back on two lines.

This patch also makes the "fatal: empty commit message?"
warning a bit less scary, since this is now a "feature"
instead of an error. However, we retain the non-zero exit
status to indicate to callers that nothing was committed.

Signed-off-by: Jeff King <[hidden email]>
---
 builtin-commit.c  |   19 ++++++++++++-------
 t/t7502-commit.sh |   11 +++++------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index f7c053a..b783e6e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -554,14 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 
  fprintf(fp,
  "\n"
- "# Please enter the commit message for your changes.\n"
- "# To abort the commit, use an empty commit message.\n"
- "# (Comment lines starting with '#' will ");
+ "# Please enter the commit message for your changes.");
  if (cleanup_mode == CLEANUP_ALL)
- fprintf(fp, "not be included)\n");
+ fprintf(fp,
+ " Lines starting\n"
+ "# with '#' will be ignored, and an empty"
+ " message aborts the commit.\n");
  else /* CLEANUP_SPACE, that is. */
- fprintf(fp, "be kept.\n"
- "# You can remove them yourself if you want to)\n");
+ fprintf(fp,
+ " Lines starting\n"
+ "# with '#' will be kept; you may remove them"
+ " yourself if you want to.\n"
+ "# An empty message aborts the commit.\n");
  if (only_include_assumed)
  fprintf(fp, "# %s\n", only_include_assumed);
 
@@ -1004,7 +1008,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
  stripspace(&sb, cleanup_mode == CLEANUP_ALL);
  if (sb.len < header_len || message_is_empty(&sb, header_len)) {
  rollback_index_files();
- die("no commit message?  aborting commit.");
+ fprintf(stderr, "Aborting commit due to empty commit message.\n");
+ exit(1);
  }
  strbuf_addch(&sb, '\0');
  if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index f111263..3eb9fae 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -141,16 +141,15 @@ test_expect_success 'cleanup commit messages (strip,-F)' '
 
 echo "sample
 
-# Please enter the commit message for your changes.
-# To abort the commit, use an empty commit message.
-# (Comment lines starting with '#' will not be included)" >expect
+# Please enter the commit message for your changes. Lines starting
+# with '#' will be ignored, and an empty message aborts the commit." >expect
 
 test_expect_success 'cleanup commit messages (strip,-F,-e)' '
 
  echo >>negative &&
  { echo;echo sample;echo; } >text &&
  git commit -e -F text -a &&
- head -n 5 .git/COMMIT_EDITMSG >actual &&
+ head -n 4 .git/COMMIT_EDITMSG >actual &&
  test_cmp expect actual
 
 '
@@ -163,7 +162,7 @@ test_expect_success 'author different from committer' '
 
  echo >>negative &&
  git commit -e -m "sample"
- head -n 8 .git/COMMIT_EDITMSG >actual &&
+ head -n 7 .git/COMMIT_EDITMSG >actual &&
  test_cmp expect actual
 '
 
@@ -182,7 +181,7 @@ test_expect_success 'committer is automatic' '
  # must fail because there is no change
  test_must_fail git commit -e -m "sample"
  ) &&
- head -n 9 .git/COMMIT_EDITMSG | \
+ head -n 8 .git/COMMIT_EDITMSG | \
  sed "s/^# Committer: .*/# Committer:/" >actual &&
  test_cmp expect actual
 '
--
1.6.0.rc1.169.g34ee

--
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 v3] Advertise the ability to abort a commit

Petr Baudis
On Thu, Jul 31, 2008 at 03:36:09AM -0400, Jeff King wrote:

>  builtin-commit.c  |   19 ++++++++++++-------
>  t/t7502-commit.sh |   11 +++++------
>  2 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/builtin-commit.c b/builtin-commit.c
> index f7c053a..b783e6e 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -554,14 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
>  
>   fprintf(fp,
>   "\n"
> - "# Please enter the commit message for your changes.\n"
> - "# To abort the commit, use an empty commit message.\n"
> - "# (Comment lines starting with '#' will ");
> + "# Please enter the commit message for your changes.");
>   if (cleanup_mode == CLEANUP_ALL)
> - fprintf(fp, "not be included)\n");
> + fprintf(fp,
> + " Lines starting\n"
> + "# with '#' will be ignored, and an empty"
> + " message aborts the commit.\n");
>   else /* CLEANUP_SPACE, that is. */
> - fprintf(fp, "be kept.\n"
> - "# You can remove them yourself if you want to)\n");
> + fprintf(fp,
> + " Lines starting\n"
> + "# with '#' will be kept; you may remove them"
> + " yourself if you want to.\n"
> + "# An empty message aborts the commit.\n");
>   if (only_include_assumed)
>   fprintf(fp, "# %s\n", only_include_assumed);
>  

This is rather funny-looking; you print _one_ fragment of the common
string by a common fprintf, but then repeat _second_ fragment of the
still-common string in a per-case fprintf. Can't we at least split this
on the line boundary, if not do something loosely like this?

                fprintf(fp,
                        "\n"
                        "# Please enter the commit message for your "
                        "changes. Lines starting\n"
                        "# with a '#' will be %s "
                        "and an empty message aborts the commit\n",
                        cleanup_mode == CLEANUP_ALL ? "ignored,"
                        /* CLEANUP_SPACE */ : "kept (you may remove them "
                                "yourself if you want to)\n#");


--
                                Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie
--
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 v3] Advertise the ability to abort a commit

Jeff King
On Thu, Jul 31, 2008 at 12:55:39PM +0200, Petr Baudis wrote:

> >   if (cleanup_mode == CLEANUP_ALL)
> > - fprintf(fp, "not be included)\n");
> > + fprintf(fp,
> > + " Lines starting\n"
> > + "# with '#' will be ignored, and an empty"
> > + " message aborts the commit.\n");
> >   else /* CLEANUP_SPACE, that is. */
> > - fprintf(fp, "be kept.\n"
> > - "# You can remove them yourself if you want to)\n");
> > + fprintf(fp,
> > + " Lines starting\n"
> > + "# with '#' will be kept; you may remove them"
> > + " yourself if you want to.\n"
> > + "# An empty message aborts the commit.\n");
> >   if (only_include_assumed)
> >   fprintf(fp, "# %s\n", only_include_assumed);
> >  
>
> This is rather funny-looking; you print _one_ fragment of the common
> string by a common fprintf, but then repeat _second_ fragment of the
> still-common string in a per-case fprintf. Can't we at least split this
> on the line boundary, if not do something loosely like this?

I just broke it by sentence, thinking that followed the semantics more
clearly (i.e., the first fprintf says one thing, then the second says
another; however, we must say the second one differently depending on
the case). I almost just split the whole paragraph by cleanup case,
allowing each to be worded and wrapped as most appropriate.

> fprintf(fp,
> "\n"
> "# Please enter the commit message for your "
> "changes. Lines starting\n"
> "# with a '#' will be %s "
> "and an empty message aborts the commit\n",
> cleanup_mode == CLEANUP_ALL ? "ignored,"
> /* CLEANUP_SPACE */ : "kept (you may remove them "
> "yourself if you want to)\n#");

I did something like that before submitting, but decided against it
because:

  - I found mine more readable, since it is hard to see in yours exactly
    where there will be a linebreak.

  - I actually changed the phrasing for the second one. Since we
    introduce another clause into the sentence in the CLEANUP_SPACE
    case, it makes sense to start another sentence for the final point.

-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