git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Altmanninger <aclopte@gmail.com>
To: Erik Cervin Edin <erik@cervined.in>, Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Johannes Altmanninger <aclopte@gmail.com>
Subject: [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish
Date: Sun, 18 Sep 2022 07:10:53 -0500	[thread overview]
Message-ID: <20220918121053.880225-1-aclopte@gmail.com> (raw)
In-Reply-To: <YyZWDkZWAkS7q+Wf@gmail.com>

Commit 68d5d03bc4 (rebase: teach --autosquash to match on sha1 in
addition to message, 2010-11-04) made --autosquash apply a commit
with subject "fixup! 012345" to the first commit in the todo list
whose OID starts with 012345. So far so good.

More recently, c44a4c650c (rebase -i: rearrange fixup/squash lines
using the rebase--helper, 2017-07-14) reimplemented this logic in C
and introduced two behavior changes.
First, OID matches are given precedence over subject prefix
matches.  Second, instead of prefix-matching OIDs, we use
lookup_commit_reference_by_name().  This means that if 012345 is a
branch name, we will apply the fixup commit to the tip of that branch
(if that is present in the todo list).

Both behavior changes might be motivated by performance concerns
(since the commit message mentions performance).  Looking through
the todo list to find a commit that matches the given prefix can be
more expensive than looking up an OID.  The runtime of the former is
of O(n*m) where n is the size of the todo list and m is the length
of a commit subject. However, if this is really a problem, we could
easily make it O(m) by constructing a trie (prefix tree).

Demonstrate both behavior changes by adding two test cases for
"fixup! foo" where foo is a commit-ish that is not an OID-prefix.
Arguably, this feature is very weird.  If no one uses it we should
consider removing it.

Regardless, there is one bad edge case to fix.  Let refspec "foo" point
to a commit with the subject "fixup! foo". Since rebase --autosquash
finds the fixup target via lookup_commit_reference_by_name(), the
fixup target is the fixup commit itself. Obviously this can't work.
We proceed with the broken invariant and drop the fixup commit
entirely.

The self-fixup was only allowed because the fixup commit was already
added to the preliminary todo list, which it shouldn't be.  Rather,
we should first compute the fixup target and only then add the fixup
commit to the todo list. Make it so, avoiding this error by design,
and add a third test for this case.

Reported-by: Erik Cervin Edin <erik@cervined.in>
Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
---
 sequencer.c                  |  4 ++--
 t/t3415-rebase-autosquash.sh | 44 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 484ca9aa50..777200a6dc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6287,8 +6287,6 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 			return error(_("the script was already rearranged."));
 		}
 
-		*commit_todo_item_at(&commit_todo, item->commit) = item;
-
 		parse_commit(item->commit);
 		commit_buffer = logmsg_reencode(item->commit, NULL, "UTF-8");
 		find_commit_subject(commit_buffer, &subject);
@@ -6355,6 +6353,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 					strhash(entry->subject));
 			hashmap_put(&subject2item, &entry->entry);
 		}
+
+		*commit_todo_item_at(&commit_todo, item->commit) = item;
 	}
 
 	if (rearranged) {
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 78c27496d6..879e628512 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -232,6 +232,50 @@ test_expect_success 'auto squash that matches longer sha1' '
 	test_line_count = 1 actual
 '
 
+test_expect_success 'auto squash that matches regex' '
+	git reset --hard base &&
+	git commit --allow-empty -m "hay needle hay" &&
+	git commit --allow-empty -m "fixup! :/[n]eedle" &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH hay needle hay # empty
+	fixup HASH fixup! :/[n]eedle # empty
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'auto squash of fixup commit that matches branch name' '
+	git reset --hard base &&
+	git commit --allow-empty -m "wip commit (just a prefix match so overshadowed by branch)" &&
+	git commit --allow-empty -m "tip of wip" &&
+	git branch wip &&
+	git commit --allow-empty -m "unrelated commit" &&
+	git commit --allow-empty -m "fixup! wip" &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH wip commit (just a prefix match so overshadowed by branch) # empty
+	pick HASH tip of wip # empty
+	fixup HASH fixup! wip # empty
+	pick HASH unrelated commit # empty
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'auto squash of fixup commit that matches branch name which points back to fixup commit' '
+	git reset --hard base &&
+	git commit --allow-empty -m "fixup! self-cycle" &&
+	git branch self-cycle &&
+	GIT_SEQUENCE_EDITOR="cat >tmp" git rebase --autosquash -i HEAD^^ &&
+	sed -ne "/^[^#]/{s/[0-9a-f]\{7,\}/HASH/g;p}" tmp >actual &&
+	cat <<-EOF >expect &&
+	pick HASH second commit
+	pick HASH fixup! self-cycle # empty
+	EOF
+	test_cmp expect actual
+'
+
 test_auto_commit_flags () {
 	git reset --hard base &&
 	echo 1 >file1 &&
-- 
2.37.3.830.gf65be7a4d6


  reply	other threads:[~2022-09-18 12:11 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-17 14:45 [BUG] fixup commit is dropped during rebase if subject = branch name Erik Cervin Edin
2022-09-17 18:04 ` Junio C Hamano
2022-09-18 14:55   ` Erik Cervin Edin
2022-09-17 23:19 ` Johannes Altmanninger
2022-09-18 12:10   ` Johannes Altmanninger [this message]
2022-09-18 15:05     ` [PATCH] sequencer: avoid dropping fixup commit that targets self via commit-ish Erik Cervin Edin
2022-09-18 17:54       ` Johannes Altmanninger
2022-09-19  1:11     ` Junio C Hamano
2022-09-19 16:07       ` Junio C Hamano
2022-09-20  3:20         ` Johannes Altmanninger
2022-09-19 16:23       ` Junio C Hamano
2022-09-20  3:11         ` [PATCH v2] " Johannes Altmanninger
2022-09-20  8:26           ` Phillip Wood
2022-09-21 18:47           ` Junio C Hamano
2022-09-22  4:00             ` Johannes Altmanninger
2022-09-22 19:32               ` Junio C Hamano
2022-09-24 22:29                 ` [PATCH v3] " Johannes Altmanninger
2022-09-19 23:09     ` [PATCH] " Junio C Hamano
2022-09-20  3:27       ` Johannes Altmanninger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220918121053.880225-1-aclopte@gmail.com \
    --to=aclopte@gmail.com \
    --cc=erik@cervined.in \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).