git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Vojtěch Knyttl" <vojtech@knyt.tl>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains
Date: Tue, 12 Jan 2021 21:49:39 +0100	[thread overview]
Message-ID: <20210112204939.1095-1-martin.agren@gmail.com> (raw)
In-Reply-To: <pull.818.v2.git.1610123298764.gitgitgadget@gmail.com>

Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
>
> We actually need to actively suppress that clean-up lest a configured
> `commit.cleanup` may interfere with what we want to do: leave the commit
> message unchanged.

>     Changes since v1:>
>      * The fix now works even if commit.cleanup = commit

Indeed. FWIW, this patch looks good to me.

There is the lone remaining user of `CLEANUP_MSG` where we're handling
the skipping of the final fixup in a chain and which is part of a larger
block of code to handle various cases like that around `git rebase
--skip`. I wrote some tests on top of your patch to see what happens and
try to understand what's going on. The tests are below.

I can't say I grok all that's going on in the implementation. By the
time we're finalizing the commit message, it seems to me that we've lost
track of which of the lines that look like comments are indeed comment
lines added by us and which actually originate in the commit messages
we're trying to rebase and which just happen to begin with a comment
character.

Maybe these tests could be simplified a bit, or de-boilerplated to some
extent, but I think they do correctly demonstrate a similar bug that
remains after your fix.

What do you think? Are these test failures useful at all? Would it be
an easy fix? In any case, I don't think the presence of these bugs need
to hold up the fix you've posted here. The bugs look like they're
related, but they are in different parts of the code (to the best of my
understanding).

FWIW, this diff is Signed-off-by me if you want to run with it. Maybe
include some part of it that you agree with in your commit?

(Yes, this diff mentions "master" several times. This file already
mentions that branch name. You have a patch in seen to address that,
but there would be a semantic conflict.)

Martin

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 88040bc435..f80318998f 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -448,4 +448,63 @@ test_expect_success 'fixup does not clean up commit message' '
 	test "$oneline" = "$(git show -s --format=%s)"
 '
 
+test_expect_failure 'fixup does not clean message (conflict in only fixup)' '
+	test_when_finished "git checkout master" &&
+	oneline="#ladder" &&
+	echo version-1 >file &&
+	git add file &&
+	git commit -m version-1 &&
+	git checkout -B fixup-topic HEAD^ &&
+	# Now make two commits (oneline+fixup) that conflict
+	# in the fixup when we rebase them onto master.
+	git commit --allow-empty -m "$oneline" &&
+	echo conflict >file &&
+	git add file &&
+	git commit --fixup HEAD &&
+	test_must_fail git -c commit.cleanup=strip rebase -ki \
+		--autosquash --onto=master HEAD~2 &&
+	git rebase --skip &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
+test_expect_failure 'fixup does not clean message (conflict in non-last fixup)' '
+	test_when_finished "git checkout master" &&
+	oneline="#not-a-comment" &&
+	echo version-2 >file &&
+	git add file &&
+	git commit -m version-2 &&
+	git checkout -B fixup-topic HEAD^ &&
+	# Now make three commits (oneline+fixup+fixup) that conflict
+	# in the first fixup when we rebase them onto master.
+	git commit --allow-empty -m "$oneline" &&
+	echo conflict >file &&
+	git add file &&
+	git commit --fixup HEAD &&
+	git commit --fixup HEAD --allow-empty &&
+	test_must_fail git -c commit.cleanup=strip rebase -ki \
+		--autosquash --onto=master HEAD~3 &&
+	git rebase --skip &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
+test_expect_failure 'fixup does not clean message (conflict in last fixup)' '
+	test_when_finished "git checkout master" &&
+	oneline="#hash" &&
+	echo version-3 >file &&
+	git add file &&
+	git commit -m version-3 &&
+	git checkout -B fixup-topic HEAD^ &&
+	# Now make three commits (oneline+fixup+fixup) that conflict
+	# in the second fixup when we rebase them onto master.
+	git commit --allow-empty -m "$oneline" &&
+	git commit --fixup HEAD --allow-empty &&
+	echo conflict >file &&
+	git add file &&
+	git commit --fixup HEAD &&
+	test_must_fail git -c commit.cleanup=strip rebase -ki \
+		--autosquash --onto=master HEAD~3 &&
+	git rebase --skip &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
 test_done

  reply	other threads:[~2021-01-13  0:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-19  0:22 [PATCH] rebase -i: do leave commit message intact in fixup! chains Johannes Schindelin via GitGitGadget
2020-12-19 14:47 ` Martin Ågren
2020-12-19 15:00   ` Johannes Schindelin
2021-01-08 16:28 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
2021-01-12 20:49   ` Martin Ågren [this message]
2021-01-28 16:19     ` Johannes Schindelin
2021-01-12 23:12   ` Junio C Hamano
2021-01-28 16:24     ` Johannes Schindelin
2021-01-28 20:18       ` Junio C Hamano
2021-01-28 16:16   ` [PATCH v3] " Johannes Schindelin via GitGitGadget

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=20210112204939.1095-1-martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=szeder.dev@gmail.com \
    --cc=vojtech@knyt.tl \
    /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).