threads / bug / 61836

bug/defaults: COMMIT_EDITMSG not reused after a failed commit

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

## tl;dr

6 messages between Jul 24, 2024 and Jul 25, 2024.

replies: 5people: 4as markdown or json

Robert Coup· Jul 24, 2024, 11:28 UTC · lore
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]
Junio C Hamano· Jul 24, 2024, 16:37 UTC · re: Robert Coup · lore

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

Robert Coup <robert.coup@koordinates.com> writes:
Show 7 quoted lines
> 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.
Konstantin Ryabitsev· Jul 24, 2024, 16:53 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 24, 2024 at 09:37:01AM GMT, Junio C Hamano wrote:
Show 21 quoted lines
> > 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
Jeff King· Jul 24, 2024, 21:08 UTC · re: Konstantin Ryabitsev · lore

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

On Wed, Jul 24, 2024 at 12:53:36PM -0400, Konstantin Ryabitsev wrote:
Show 5 quoted lines
> 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
Junio C Hamano· Jul 25, 2024, 15:21 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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.

Show 92 quoted lines
> -- >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' '
Robert Coup· Jul 25, 2024, 08:15 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Wed, 24 Jul 2024 at 17:37, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>
> 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 :)

← back to recent threads