[PATCH 0/2] commit: preserve commit hash on a no-op amend

[PATCH 0/2] commit: preserve commit hash on a no-op amend

[Harald Nordgren via GitGitGadget]

git commit --amend --no-edit rewrote the commit and moved the branch tip
even when nothing changed, because the committer date was reset to "now".
Reuse the existing committer date so a no-op amend keeps the commit hash and
leaves the branch untouched.

A real change (tree, message, author, committer, or signing) still rewrites
as before.

Harald Nordgren (2):
  commit: extract commit_index_files_or_die() helper
  commit: keep the commit on a no-op amend

 Documentation/git-commit.adoc         |   6 ++
 builtin/commit.c                      |  69 ++++++++++++++-
 t/t7501-commit-basic-functionality.sh | 119 ++++++++++++++++++++++++++
 3 files changed, 190 insertions(+), 4 deletions(-)


base-commit: ea97ad8d017de0c9037451a78008a0fd60abea0c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2334%2FHaraldNordgren%2Famend-noop-keeps-commit-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2334/HaraldNordgren/amend-noop-keeps-commit-v1
Pull-Request: https://github.com/git/git/pull/2334
-- 
gitgitgadget

[PATCH 1/2] commit: extract commit_index_files_or_die() helper

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

A later change adds a second caller that commits the index lock and dies
on failure, so wrap that into a helper to avoid duplicating its message.

No functional change intended.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 builtin/commit.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 28f6174503..1a51450660 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -248,6 +248,14 @@ static int commit_index_files(void)
 	return err;
 }
 
+static void commit_index_files_or_die(void)
+{
+	if (commit_index_files())
+		die(_("repository has been updated, but unable to write\n"
+		      "new index file. Check that disk is not full and quota is\n"
+		      "not exceeded, and then \"git restore --staged :/\" to recover."));
+}
+
 /*
  * Take a union of paths in the index and the named tree (typically, "HEAD"),
  * and return the paths that match the given pattern in list.
@@ -1954,10 +1962,7 @@ int cmd_commit(int argc,
 	unlink(git_path_merge_mode(the_repository));
 	unlink(git_path_squash_msg(the_repository));
 
-	if (commit_index_files())
-		die(_("repository has been updated, but unable to write\n"
-		      "new index file. Check that disk is not full and quota is\n"
-		      "not exceeded, and then \"git restore --staged :/\" to recover."));
+	commit_index_files_or_die();
 
 	git_test_write_commit_graph_or_die(the_repository->objects->sources);
 
-- 
gitgitgadget

[PATCH 2/2] commit: keep the commit on a no-op amend

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

"git commit --amend --no-edit" reset the committer date to "now" and
rewrote the commit even when nothing else changed, moving the branch tip
to a new hash for an effective no-op.

Build the amended commit reusing the existing committer date: if that
reproduces the current commit, leave the branch alone, report "nothing
to amend", and skip the reflog entry and the post-commit and post-rewrite
hooks.

Signing always rewrites the commit, since its signature cannot reproduce
the original, so it skips this detection.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 Documentation/git-commit.adoc         |   6 ++
 builtin/commit.c                      |  56 ++++++++++++
 t/t7501-commit-basic-functionality.sh | 119 ++++++++++++++++++++++++++
 3 files changed, 181 insertions(+)

diff --git a/Documentation/git-commit.adoc b/Documentation/git-commit.adoc
index 8329c1034b..c433c60929 100644
--- a/Documentation/git-commit.adoc
+++ b/Documentation/git-commit.adoc
@@ -282,6 +282,12 @@ variable (see linkgit:git-config[1]).
 	parents and author as the current one (the `--reset-author`
 	option can countermand this).
 +
+If the amended commit would be identical to the original (its tree,
+message, author, parents, and committer are all unchanged), the original
+committer date is kept so that the commit, and thus the branch tip, is
+left untouched. A commit that is being signed (`-S`, or `commit.gpgsign`)
+is always rewritten, since its signature cannot reproduce the original.
++
 --
 It is a rough equivalent for:
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 1a51450660..e330a53d5c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -17,6 +17,7 @@
 #include "dir.h"
 #include "editor.h"
 #include "environment.h"
+#include "ident.h"
 #include "diff.h"
 #include "commit.h"
 #include "add-interactive.h"
@@ -760,6 +761,49 @@ static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 	repo_unuse_commit_buffer(the_repository, commit, buffer);
 }
 
+/*
+ * Rebuild the amended commit reusing the existing committer date and report
+ * whether it reproduces the current commit. Because the committer date is the
+ * only field that an amend would otherwise replace with "now", an exact match
+ * means everything else (tree, message, author, parents, committer identity)
+ * is unchanged too.
+ */
+static int amend_is_noop(struct commit *current_head,
+				 const struct strbuf *message,
+				 const struct commit_list *parents,
+				 const char *author,
+				 const struct commit_extra_header *extra,
+				 struct object_id *oid)
+{
+	const char *buffer, *committer_line;
+	size_t len;
+	struct ident_split ident;
+	struct strbuf date = STRBUF_INIT;
+	int unchanged = 0;
+
+	buffer = repo_get_commit_buffer(the_repository, current_head, NULL);
+	committer_line = find_commit_header(buffer, "committer", &len);
+	if (committer_line && !split_ident_line(&ident, committer_line, len) &&
+	    ident.date_begin) {
+		const char *committer;
+
+		strbuf_add(&date, ident.date_begin,
+			   ident.tz_end - ident.date_begin);
+		committer = fmt_ident(getenv("GIT_COMMITTER_NAME"),
+				      getenv("GIT_COMMITTER_EMAIL"),
+				      WANT_COMMITTER_IDENT, date.buf,
+				      IDENT_STRICT);
+		if (!commit_tree_extended(message->buf, message->len,
+					  &the_repository->index->cache_tree->oid,
+					  parents, oid, author, committer, NULL,
+					  extra))
+			unchanged = oideq(oid, &current_head->object.oid);
+	}
+	repo_unuse_commit_buffer(the_repository, current_head, buffer);
+	strbuf_release(&date);
+	return unchanged;
+}
+
 static void change_data_free(void *util, const char *str UNUSED)
 {
 	struct wt_status_change_data *d = util;
@@ -1943,6 +1987,18 @@ int cmd_commit(int argc,
 		append_merge_tag_headers(parents, &tail);
 	}
 
+	if (amend && current_head && !sign_commit &&
+	    amend_is_noop(current_head, &sb, parents, author_ident.buf,
+			  extra, &oid)) {
+		commit_index_files_or_die();
+		if (!quiet)
+			fprintf(stderr,
+				_("nothing to amend; %s left unchanged\n"),
+				repo_find_unique_abbrev(the_repository, &oid,
+							DEFAULT_ABBREV));
+		goto cleanup;
+	}
+
 	if (commit_tree_extended(sb.buf, sb.len, &the_repository->index->cache_tree->oid,
 				 parents, &oid, author_ident.buf, NULL,
 				 sign_commit, extra)) {
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index a37509f004..160edb9c0a 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -11,6 +11,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-diff.sh"
+. "$TEST_DIRECTORY/lib-gpg.sh"
 
 author='The Real Author <someguy@his.email.org>'
 
@@ -654,6 +655,124 @@ test_expect_success 'amend commit to fix author' '
 
 '
 
+test_expect_success 'amend --no-edit that changes nothing keeps the commit' '
+	git reset --hard &&
+	old=$(git rev-parse HEAD) &&
+	test_tick &&
+	git commit --amend --no-edit 2>err &&
+	test_cmp_rev $old HEAD &&
+	test_grep "nothing to amend" err
+'
+
+test_expect_success 'amend --no-edit keeps the commit out of the reflog' '
+	git reset --hard &&
+	git rev-parse HEAD@{0} >before &&
+	test_tick &&
+	git commit --amend --no-edit &&
+	git rev-parse HEAD@{0} >after &&
+	test_cmp before after
+'
+
+test_expect_success 'amend --signoff is idempotent once signed off' '
+	git reset --hard &&
+	test_tick &&
+	git commit --amend --no-edit --signoff &&
+	signed=$(git rev-parse HEAD) &&
+	git log -1 --format=%B | grep "^Signed-off-by:" &&
+	test_tick &&
+	git commit --amend --no-edit --signoff &&
+	test_cmp_rev $signed HEAD
+'
+
+test_expect_success 'amend that changes the tree still rewrites the commit' '
+	git reset --hard &&
+	old=$(git rev-parse HEAD) &&
+	echo changed >>file &&
+	git add file &&
+	test_tick &&
+	git commit --amend --no-edit &&
+	test_cmp_rev ! $old HEAD
+'
+
+test_expect_success 'amend that changes the committer still rewrites the commit' '
+	git reset --hard &&
+	old=$(git rev-parse HEAD) &&
+	test_tick &&
+	GIT_COMMITTER_EMAIL=other@example.com \
+		git commit --amend --no-edit &&
+	test_cmp_rev ! $old HEAD
+'
+
+test_expect_success 'amend that changes only the message still rewrites the commit' '
+	git reset --hard &&
+	old=$(git rev-parse HEAD) &&
+	test_tick &&
+	git commit --amend -m "new message" &&
+	test_cmp_rev ! $old HEAD &&
+	echo "new message" >expect &&
+	git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'amend --allow-empty of an empty commit that changes nothing keeps it' '
+	test_when_finished "git reset --hard parent && git tag -d parent" &&
+	git tag parent &&
+	git commit --allow-empty -m "empty" &&
+	old=$(git rev-parse HEAD) &&
+	test_tick &&
+	git commit --amend --no-edit --allow-empty 2>err &&
+	test_cmp_rev $old HEAD &&
+	test_grep "nothing to amend" err
+'
+
+test_expect_success GPG 'amend --no-edit of a signed commit is not a no-op' '
+	git reset --hard &&
+	test_tick &&
+	git commit --amend --no-edit -S &&
+	signed=$(git rev-parse HEAD) &&
+	git verify-commit HEAD &&
+	test_tick &&
+	git commit --amend --no-edit -S &&
+	test_cmp_rev ! $signed HEAD &&
+	git verify-commit HEAD
+'
+
+test_expect_success GPG 'amend --no-edit with commit.gpgsign is not a no-op' '
+	git reset --hard &&
+	test_tick &&
+	old=$(git rev-parse HEAD) &&
+	git -c commit.gpgsign=true commit --amend --no-edit &&
+	test_cmp_rev ! $old HEAD &&
+	git verify-commit HEAD
+'
+
+test_expect_success 'amend --reset-author rewrites the commit' '
+	git reset --hard &&
+	old=$(git rev-parse HEAD) &&
+	test_tick &&
+	git commit --amend --no-edit --reset-author &&
+	test_cmp_rev ! $old HEAD
+'
+
+test_expect_success 'amend --date rewrites the commit' '
+	git reset --hard &&
+	old=$(git rev-parse HEAD) &&
+	test_tick &&
+	git commit --amend --no-edit --date="@1234567890 +0000" &&
+	test_cmp_rev ! $old HEAD
+'
+
+test_expect_success 'amend that changes nothing skips the post-commit hook' '
+	test_when_finished "rm -f post-commit.ran" &&
+	test_hook post-commit <<-\EOF &&
+	>post-commit.ran
+	EOF
+	git reset --hard &&
+	test_tick &&
+	git commit --amend --no-edit &&
+	test_path_is_missing post-commit.ran
+'
+
 test_expect_success 'git commit <file> with dirty index' '
 	echo tacocat >elif &&
 	echo tehlulz >chz &&
-- 
gitgitgadget

Re: [PATCH 0/2] commit: preserve commit hash on a no-op amend

[Johannes Sixt]

Am 13.06.26 um 11:16 schrieb Harald Nordgren via GitGitGadget:
> git commit --amend --no-edit rewrote the commit and moved the branch tip
> even when nothing changed, because the committer date was reset to "now".
> Reuse the existing committer date so a no-op amend keeps the commit hash and
> leaves the branch untouched.

`git commit --amend --no-edit` is a way to set the committer timestamp
to the current time without changing other aspects of the commit. This
takes away this ability, doesn't it?

Is this keyed to --no-edit? Why is this mode special? Wouldn't it be an
identical case when the commit message is passed to the editor, but
comes back unchanged?

An invocation of `git commit` asks to "please make a new commit". But in
the suggested mode, no new commit is created. Shouldn't this then be
regarded as failure?

What happens with the current branch? Is it left unchanged (no ref
update occurs) or is it changed (a ref update occurs, but it happens to
be a no-op)? And does this then generate a reflog entry?

The updated documentation says about signed commits (note: I am totally
clueless about commit signing procedures):

> A commit that is being signed (`-S`, or `commit.gpgsign`)
> is always rewritten, since its signature cannot reproduce the original.

But if the commit doesn't change in any way, why should the signature be
invalidated, rewritten, or updated?

-- Hannes