bug/defaults: COMMIT_EDITMSG not reused after a failed commit

bug/defaults: COMMIT_EDITMSG not reused after a failed commit

[Robert Coup]

Hi,

* What did you do before the bug happened? (Steps to reproduce your issue)

I have commit signing configured, using 1Password as an ssh-signer.

    [gpg]
        format = ssh

    [gpg "ssh"]
        program = "/Applications/1Password.app/Contents/MacOS/op-ssh-sign"

Sometimes signing a commit fails, because 1PW has updated and needs restarted
or something, I haven't been motivated to figure it out. But that's not the
issue, there could be many reasons a commit fails.

    $ git commit
    <editor opens, write beautiful commit message prose>

    error: 1Password: Could not connect to socket. Is the agent running?

    fatal: failed to write commit object

If I restart 1Password, and do `git commit` again (signing works), my previous
commit message is wiped and I need to start afresh.

* What did you expect to happen? (Expected behavior)

If a commit fails for whatever reason, I expect to be able to re-commit and it
would reuse my carefully crafted message.

* What happened instead? (Actual behavior)

It got dropped on the floor and I needed to rewrite the whole thing.

* What's different between what you expected and what actually happened?
* Anything else you want to add:

The git-commit docs state:

    $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.

So, yes, technically I can copy it or use `git commit -F .git/COMMIT_EDITMSG`
but this seems like a distinctly unhelpful default behaviour to me. Thoughts:

- descriptive commit messages are good
- most commits succeed
- if a commit fails, the user _wanted_ to commit, so why would they _want_ to
  drop the message they've previously written?
- even if a commit fails and the user does something completely different and
  now wants a different message, deleting a chunk of text is vastly easier than
  rewriting something.

While successful commits also leave the previous commit message in
.git/COMMIT_EDITMSG, this is _not_ documented behaviour (it only talks about a
"commit in progress"); so feels like we could change it:

1. delete COMMIT_EDITMSG on success
2. reopen COMMIT_EDITMSG on commit if it exists. Maybe logging something like
   "Restoring previous in-progress commit message..." might explain what's
   happening.
3. if COMMIT_EDITMSG doesn't exist, re-populate from the template before opening
   the editor. We could also do this for "parsed-as-empty" commit messages.

I just don't see any particular upside to the current default behaviour?

Rob :)


[System Info]
git version:
git version 2.39.3 (Apple Git-146)
cpu: arm64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:12:58
PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6000 arm64
compiler info: clang: 15.0.0 (clang-1500.3.9.4)
libc info: no libc information available
$SHELL (typically, interactive shell): /opt/homebrew/bin/zsh

[Enabled Hooks]

Re: bug/defaults: COMMIT_EDITMSG not reused after a failed commit

[Junio C Hamano]

Robert Coup <robert.coup@koordinates.com> writes:

> 1. delete COMMIT_EDITMSG on success
>
> 2. reopen COMMIT_EDITMSG on commit if it exists. Maybe logging something like
>    "Restoring previous in-progress commit message..." might explain what's
>    happening.
> 3. if COMMIT_EDITMSG doesn't exist, re-populate from the template before opening
>    the editor. We could also do this for "parsed-as-empty" commit messages.

Unconditionally doing this change would be disruptive to workflows
of existing users.  To them, Git left COMMIT_EDITMSG available even
after the commit to them almost forever, but suddenly it stops doing
so.  Like "git cherry-pick|rebase|revert" that got stopped can be
restarted _with_ some state information with "--continue", offering
this as an optional feature might be a possibility, but I haven't
thought things through.

An obvious and a lot more lightweight first step is to make it clear
(perhaps in the error message after a failed commit---after all,
such a failure from "git commit" should be a rare event) where you
can resurrect the draft commit message from.  That is independent
and orthogonal to the "let's reuse COMMIT_EDITMSG file" change.

A similar issue was reported a few years ago but without any
response or action.

https://lore.kernel.org/git/CAJ2_uEOk8xoLvK8B8PYc0_=kA8W_LqKwGyhKghemQDdRzA2nFA@mail.gmail.com/

Let's see if we find somebody interested in it this time.

Thanks.

Re: bug/defaults: COMMIT_EDITMSG not reused after a failed commit

[Konstantin Ryabitsev]

On Wed, Jul 24, 2024 at 09:37:01AM GMT, Junio C Hamano wrote:
> > 1. delete COMMIT_EDITMSG on success
> >
> > 2. reopen COMMIT_EDITMSG on commit if it exists. Maybe logging something like
> >    "Restoring previous in-progress commit message..." might explain what's
> >    happening.
> > 3. if COMMIT_EDITMSG doesn't exist, re-populate from the template before opening
> >    the editor. We could also do this for "parsed-as-empty" commit messages.
> 
> Unconditionally doing this change would be disruptive to workflows
> of existing users.  To them, Git left COMMIT_EDITMSG available even
> after the commit to them almost forever, but suddenly it stops doing
> so.  Like "git cherry-pick|rebase|revert" that got stopped can be
> restarted _with_ some state information with "--continue", offering
> this as an optional feature might be a possibility, but I haven't
> thought things through.
> 
> An obvious and a lot more lightweight first step is to make it clear
> (perhaps in the error message after a failed commit---after all,
> such a failure from "git commit" should be a rare event) where you
> can resurrect the draft commit message from.  That is independent
> and orthogonal to the "let's reuse COMMIT_EDITMSG file" change.

Yes, I would say even doing the following would result in a better experience
for users who don't know about .git/COMMIT_EDITMSG:

1. when git-commit fails, save the message as .git/FAILED_COMMIT_MSG
2. output "Commit message saved as .git/FAILED_COMMIT_MSG"

(exact wording/naming up for debate)

-K

Re: bug/defaults: COMMIT_EDITMSG not reused after a failed commit

[Jeff King]

On Wed, Jul 24, 2024 at 12:53:36PM -0400, Konstantin Ryabitsev wrote:

> Yes, I would say even doing the following would result in a better experience
> for users who don't know about .git/COMMIT_EDITMSG:
> 
> 1. when git-commit fails, save the message as .git/FAILED_COMMIT_MSG
> 2. output "Commit message saved as .git/FAILED_COMMIT_MSG"

I proposed something like (2) long ago. I'll reproduce the (rebased
forward) patch below, but here's the original thread with a little bit
of discussion:

  https://lore.kernel.org/git/20120723185218.GC27588@sigill.intra.peff.net/

It just told you about COMMIT_EDITMSG, making it your responsibility to
recover it before running "git commit" again. Your (1) makes it a little
nicer, in that you can run "git commit" and then pull the content from
the other file into your editor. Or we could even provide an option to
pre-populate the message with it.

Junio was lukewarm on the original, so I'm not sure why I've been
holding on to it all these years. But maybe it would help as a guide for
anybody who wants to work on what you've proposed above.

-- >8 --
From: Jeff King <peff@peff.net>
Date: Mon, 23 Jul 2012 14:52:18 -0400
Subject: [PATCH] commit: give a hint when a commit message has been abandoned

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>
---
 builtin/commit.c                          | 15 +++++++++++++++
 t/t7500-commit-template-squash-signoff.sh |  3 +--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dec78dfb86..42fefaa0e3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -160,6 +160,16 @@ static int opt_parse_porcelain(const struct option *opt, const char *arg, int un
 	return 0;
 }
 
+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;
@@ -1090,6 +1100,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			exit(1);
 		}
 		strvec_clear(&env);
+		atexit(maybe_mention_abandoned_message);
+		mention_abandoned_message = 1;
 	}
 
 	if (!no_verify &&
@@ -1813,11 +1825,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) {
 		rollback_index_files();
 		fprintf(stderr, _("Aborting commit due to empty commit message.\n"));
+		mention_abandoned_message = 0;
 		exit(1);
 	}
 	if (template_untouched(&sb, template_file, cleanup_mode) && !allow_empty_message) {
 		rollback_index_files();
 		fprintf(stderr, _("Aborting commit; you did not edit the message.\n"));
+		mention_abandoned_message = 0;
 		exit(1);
 	}
 
@@ -1855,6 +1869,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		die("%s", err.buf);
 	}
 
+	mention_abandoned_message = 0;
 	sequencer_post_commit_cleanup(the_repository, 0);
 	unlink(git_path_merge_head(the_repository));
 	unlink(git_path_merge_msg(the_repository));
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a7..c476a26235 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -396,13 +396,12 @@ test_expect_success 'consecutive amend! commits remove amend! line from commit m
 
 test_expect_success 'deny to create amend! commit if its commit msg body is empty' '
 	commit_for_rebase_autosquash_setup &&
-	echo "Aborting commit due to empty commit message body." >expected &&
 	(
 		set_fake_editor &&
 		test_must_fail env FAKE_COMMIT_MESSAGE="amend! target message subject line" \
 			git commit --fixup=amend:HEAD~ 2>actual
 	) &&
-	test_cmp expected actual
+	grep "Aborting commit due to empty commit message body" actual
 '
 
 test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '
-- 
2.46.0.rc1.447.g578b9b2b5c

Re: bug/defaults: COMMIT_EDITMSG not reused after a failed commit

[Robert Coup]

Hi Junio,

On Wed, 24 Jul 2024 at 17:37, Junio C Hamano <gitster@pobox.com> wrote:
>
> Unconditionally doing this change would be disruptive to workflows
> of existing users.  To them, Git left COMMIT_EDITMSG available even
> after the commit to them almost forever, but suddenly it stops doing
> so.

A general question: how far down the "I can imagine a hypothetical
workflow" route do we need to go? Moreso when the behaviour is
documented as doing something different, and it's noted in the list
archive as a bug? I appreciate there's a lot of users out there who do
a lot of weird and wonderful things. Could it suffice for the
hypothetical user to have an opt-in way to get to the old behaviour?

Some experimenting reveals a simple `git commit -F
.git/COMMIT_EDITMSG` doesn't work, since the comments get committed;
and using `git commit --template .git/COMMIT_EDITMSG` repeats the
#boilerplate, and results in an "Aborting commit; you did not edit the
message." error, even when you do. `git commit --edit -F
.git/COMMIT_EDITMSG --cleanup=strip` works, except it also repeats the
#boilerplate again, and it's getting unwieldy. I'll explore Jeff's
patch too.

Thanks,

Rob :)

Re: bug/defaults: COMMIT_EDITMSG not reused after a failed commit

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It just told you about COMMIT_EDITMSG, making it your responsibility to
> recover it before running "git commit" again. Your (1) makes it a little
> nicer, in that you can run "git commit" and then pull the content from
> the other file into your editor. Or we could even provide an option to
> pre-populate the message with it.
>
> Junio was lukewarm on the original, so I'm not sure why I've been
> holding on to it all these years. But maybe it would help as a guide for
> anybody who wants to work on what you've proposed above.

I think it was only me being allergic of the use of atexit() for a
narrow single purpose, and perhaps I was hoping that we might be
able to come up with a bit more generalized interface, possibly
based on atexit(), to register common cleanup "hooks", as back then
we only had a handful of calls to atexit() in mid 2012, and I was
worried that we may add a lot more of them in an uncontrolled way.

> -- >8 --
> From: Jeff King <peff@peff.net>
> Date: Mon, 23 Jul 2012 14:52:18 -0400
> Subject: [PATCH] commit: give a hint when a commit message has been abandoned
>
> 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>
> ---
>  builtin/commit.c                          | 15 +++++++++++++++
>  t/t7500-commit-template-squash-signoff.sh |  3 +--
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index dec78dfb86..42fefaa0e3 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -160,6 +160,16 @@ static int opt_parse_porcelain(const struct option *opt, const char *arg, int un
>  	return 0;
>  }
>  
> +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;
> @@ -1090,6 +1100,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			exit(1);
>  		}
>  		strvec_clear(&env);
> +		atexit(maybe_mention_abandoned_message);
> +		mention_abandoned_message = 1;
>  	}
>  
>  	if (!no_verify &&
> @@ -1813,11 +1825,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	if (message_is_empty(&sb, cleanup_mode) && !allow_empty_message) {
>  		rollback_index_files();
>  		fprintf(stderr, _("Aborting commit due to empty commit message.\n"));
> +		mention_abandoned_message = 0;
>  		exit(1);
>  	}
>  	if (template_untouched(&sb, template_file, cleanup_mode) && !allow_empty_message) {
>  		rollback_index_files();
>  		fprintf(stderr, _("Aborting commit; you did not edit the message.\n"));
> +		mention_abandoned_message = 0;
>  		exit(1);
>  	}
>  
> @@ -1855,6 +1869,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		die("%s", err.buf);
>  	}
>  
> +	mention_abandoned_message = 0;
>  	sequencer_post_commit_cleanup(the_repository, 0);
>  	unlink(git_path_merge_head(the_repository));
>  	unlink(git_path_merge_msg(the_repository));
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 4dca8d97a7..c476a26235 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -396,13 +396,12 @@ test_expect_success 'consecutive amend! commits remove amend! line from commit m
>  
>  test_expect_success 'deny to create amend! commit if its commit msg body is empty' '
>  	commit_for_rebase_autosquash_setup &&
> -	echo "Aborting commit due to empty commit message body." >expected &&
>  	(
>  		set_fake_editor &&
>  		test_must_fail env FAKE_COMMIT_MESSAGE="amend! target message subject line" \
>  			git commit --fixup=amend:HEAD~ 2>actual
>  	) &&
> -	test_cmp expected actual
> +	grep "Aborting commit due to empty commit message body" actual
>  '
>  
>  test_expect_success 'amend! commit allows empty commit msg body with --allow-empty-message' '