git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] rebase -i: do leave commit message intact in fixup! chains
@ 2020-12-19  0:22 Johannes Schindelin via GitGitGadget
  2020-12-19 14:47 ` Martin Ågren
  2021-01-08 16:28 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
  0 siblings, 2 replies; 6+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-12-19  0:22 UTC (permalink / raw)
  To: git
  Cc: Vojtěch Knyttl, SZEDER Gábor, Johannes Schindelin,
	Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
'squash' commands, 2017-01-02), this developer introduced a change of
behavior by mistake: when encountering a `fixup!` commit (or multiple
`fixup!` commits) without any `squash!` commit thrown in, the final `git
commit` was invoked with `--cleanup=strip`. Prior to that commit, the
commit command had been called without that `--cleanup` option.

Since we explicitly read the original commit message from a file in that
case, there is really no sense in forcing that clean-up.

Reported-by: Vojtěch Knyttl <vojtech@knyt.tl>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Fix bug in interactive rebases where fixup! cleans up the commit message

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/818

 sequencer.c                  | 5 ++---
 t/t3415-rebase-autosquash.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a467700..749bddd7a1f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2001,10 +2001,9 @@ static int do_pick_commit(struct repository *r,
 		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
-		else if (file_exists(rebase_path_fixup_msg())) {
-			flags |= CLEANUP_MSG;
+		else if (file_exists(rebase_path_fixup_msg()))
 			msg_file = rebase_path_fixup_msg();
-		} else {
+		else {
 			const char *dest = git_path_squash_msg(r);
 			unlink(dest);
 			if (copy_file(dest, rebase_path_squash_msg(), 0666))
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 7bab6000dc7..4c83c98b3fc 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '
 	test XZWY = $(git show | tr -cd W-Z)
 '
 
+test_expect_success 'fixup does not clean up commit message' '
+	oneline="#818" &&
+	git commit --allow-empty -m "$oneline" &&
+	git commit --fixup HEAD --allow-empty &&
+	git rebase -ki --autosquash HEAD~2 &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
 test_done

base-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rebase -i: do leave commit message intact in fixup! chains
  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
  1 sibling, 1 reply; 6+ messages in thread
From: Martin Ågren @ 2020-12-19 14:47 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: Git Mailing List, Vojtěch Knyttl, SZEDER Gábor,
	Johannes Schindelin

Hi Dscho,

On Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
> 'squash' commands, 2017-01-02), this developer introduced a change of
> behavior by mistake: when encountering a `fixup!` commit (or multiple
> `fixup!` commits) without any `squash!` commit thrown in, the final `git
> commit` was invoked with `--cleanup=strip`. Prior to that commit, the
> commit command had been called without that `--cleanup` option.
>
> Since we explicitly read the original commit message from a file in that
> case, there is really no sense in forcing that clean-up.

>                 if (!final_fixup)
>                         msg_file = rebase_path_squash_msg();
> -               else if (file_exists(rebase_path_fixup_msg())) {
> -                       flags |= CLEANUP_MSG;
> +               else if (file_exists(rebase_path_fixup_msg()))
>                         msg_file = rebase_path_fixup_msg();
> -               } else {
> +               else {

I see. The bug survived your 789b3effec ("sequencer: make commit
options more extensible", 2017-03-23). Which isn't surprising for such a
mechanical change.

Nit: The "else" still needs braces, so if we follow the coding
guidelines, the "else if" should also use them. And even the "if",
FWIW. So it would arguably be more in line with CodingGuidelines to have
this diff just drop a single line, no additions needed.

So what this does in the end is, it stops adding `--cleanup=strip` and
it doesn't do anything instead, i.e., not even `--cleanup=whitespace`.
OK, we want to use the exact original message. But what if
`commit.cleanup` happens to be "strip"?

> +test_expect_success 'fixup does not clean up commit message' '
> +       oneline="#818" &&
> +       git commit --allow-empty -m "$oneline" &&
> +       git commit --fixup HEAD --allow-empty &&
> +       git rebase -ki --autosquash HEAD~2 &&
> +       test "$oneline" = "$(git show -s --format=%s)"
> +'

I changed your test to use

  git -c commit.cleanup=strip rebase ...

and it started failing. Maybe `run_git_command()` in sequencer.c could
learn to pass `--cleanup=verbatim` or in some other way make sure to
override any user configuration here? I couldn't figure out how to get
this to actually work, though...

Looking around for `CLEANUP_MSG`, I spotted the logic added by
15ef69314d ("rebase --skip: clean up commit message after a failed
fixup/squash", 2018-04-27). It seems like it has the same problem, but
that this proposed patch misses it. I did some testing that seemed to
confirm it:

Adding a commit with some "#message", then adding a fixup and then
adding a fixup that will conflict, then running the rebase and skipping
the conflicting fixup, I end up with a commit with the empty log
message. That's both before and after this proposed patch.

Martin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rebase -i: do leave commit message intact in fixup! chains
  2020-12-19 14:47 ` Martin Ågren
@ 2020-12-19 15:00   ` Johannes Schindelin
  0 siblings, 0 replies; 6+ messages in thread
From: Johannes Schindelin @ 2020-12-19 15:00 UTC (permalink / raw)
  To: Martin Ågren
  Cc: Johannes Schindelin via GitGitGadget, Git Mailing List,
	Vojtěch Knyttl, SZEDER Gábor

[-- Attachment #1: Type: text/plain, Size: 3405 bytes --]

Hi Martin,

On Sat, 19 Dec 2020, Martin Ågren wrote:

> On Sat, 19 Dec 2020 at 01:25, Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
> > 'squash' commands, 2017-01-02), this developer introduced a change of
> > behavior by mistake: when encountering a `fixup!` commit (or multiple
> > `fixup!` commits) without any `squash!` commit thrown in, the final `git
> > commit` was invoked with `--cleanup=strip`. Prior to that commit, the
> > commit command had been called without that `--cleanup` option.
> >
> > Since we explicitly read the original commit message from a file in that
> > case, there is really no sense in forcing that clean-up.
>
> >                 if (!final_fixup)
> >                         msg_file = rebase_path_squash_msg();
> > -               else if (file_exists(rebase_path_fixup_msg())) {
> > -                       flags |= CLEANUP_MSG;
> > +               else if (file_exists(rebase_path_fixup_msg()))
> >                         msg_file = rebase_path_fixup_msg();
> > -               } else {
> > +               else {
>
> I see. The bug survived your 789b3effec ("sequencer: make commit
> options more extensible", 2017-03-23). Which isn't surprising for such a
> mechanical change.
>
> Nit: The "else" still needs braces, so if we follow the coding
> guidelines, the "else if" should also use them. And even the "if",
> FWIW. So it would arguably be more in line with CodingGuidelines to have
> this diff just drop a single line, no additions needed.
>
> So what this does in the end is, it stops adding `--cleanup=strip` and
> it doesn't do anything instead, i.e., not even `--cleanup=whitespace`.
> OK, we want to use the exact original message. But what if
> `commit.cleanup` happens to be "strip"?
>
> > +test_expect_success 'fixup does not clean up commit message' '
> > +       oneline="#818" &&
> > +       git commit --allow-empty -m "$oneline" &&
> > +       git commit --fixup HEAD --allow-empty &&
> > +       git rebase -ki --autosquash HEAD~2 &&
> > +       test "$oneline" = "$(git show -s --format=%s)"
> > +'
>
> I changed your test to use
>
>   git -c commit.cleanup=strip rebase ...
>
> and it started failing. Maybe `run_git_command()` in sequencer.c could
> learn to pass `--cleanup=verbatim` or in some other way make sure to
> override any user configuration here? I couldn't figure out how to get
> this to actually work, though...
>
> Looking around for `CLEANUP_MSG`, I spotted the logic added by
> 15ef69314d ("rebase --skip: clean up commit message after a failed
> fixup/squash", 2018-04-27). It seems like it has the same problem, but
> that this proposed patch misses it. I did some testing that seemed to
> confirm it:
>
> Adding a commit with some "#message", then adding a fixup and then
> adding a fixup that will conflict, then running the rebase and skipping
> the conflicting fixup, I end up with a commit with the empty log
> message. That's both before and after this proposed patch.

Thank you so much for this detailed feedback. I will take care of it next
year, after taking a semi-vacation interrupted only by releasing -rc1
(check), -rc2 (TBD) and v2.30.0 final (TBD).

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] rebase -i: do leave commit message intact in fixup! chains
  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
@ 2021-01-08 16:28 ` Johannes Schindelin via GitGitGadget
  2021-01-12 20:49   ` Martin Ågren
  2021-01-12 23:12   ` Junio C Hamano
  1 sibling, 2 replies; 6+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-08 16:28 UTC (permalink / raw)
  To: git
  Cc: Vojtěch Knyttl, SZEDER Gábor, Martin Ågren,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
'squash' commands, 2017-01-02), this developer introduced a change of
behavior by mistake: when encountering a `fixup!` commit (or multiple
`fixup!` commits) without any `squash!` commit thrown in, the final `git
commit` was invoked with `--cleanup=strip`. Prior to that commit, the
commit command had been called without that `--cleanup` option.

Since we explicitly read the original commit message from a file in that
case, there is really no sense in forcing that clean-up.

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.

Reported-by: Vojtěch Knyttl <vojtech@knyt.tl>
Helped-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Fix bug in interactive rebases where fixup! cleans up the commit message
    
    Original report here:
    https://lore.kernel.org/git/CANVGpwZGbzYLMeMze64e_OU9p3bjyEgzC5thmNBr6LttBt%2BYGw%40mail.gmail.com/t
    
    Changes since v1:
    
     * The fix now works even if commit.cleanup = commit

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/818

Range-diff vs v1:

 1:  bfade3b146f ! 1:  c760d6cd203 rebase -i: do leave commit message intact in fixup! chains
     @@ Commit message
          Since we explicitly read the original commit message from a file in that
          case, there is really no sense in forcing that clean-up.
      
     +    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.
     +
          Reported-by: Vojtěch Knyttl <vojtech@knyt.tl>
     +    Helped-by: Martin Ågren <martin.agren@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## sequencer.c ##
     +@@ sequencer.c: N_("you have staged changes in your working tree\n"
     + #define CLEANUP_MSG (1<<3)
     + #define VERIFY_MSG  (1<<4)
     + #define CREATE_ROOT_COMMIT (1<<5)
     ++#define VERBATIM_MSG (1<<6)
     + 
     + static int run_command_silent_on_success(struct child_process *cmd)
     + {
     +@@ sequencer.c: static int run_git_commit(const char *defmsg,
     + 		strvec_pushl(&cmd.args, "-C", "HEAD", NULL);
     + 	if ((flags & CLEANUP_MSG))
     + 		strvec_push(&cmd.args, "--cleanup=strip");
     ++	if ((flags & VERBATIM_MSG))
     ++		strvec_push(&cmd.args, "--cleanup=verbatim");
     + 	if ((flags & EDIT_MSG))
     + 		strvec_push(&cmd.args, "-e");
     + 	else if (!(flags & CLEANUP_MSG) &&
     +@@ sequencer.c: static int try_to_commit(struct repository *r,
     + 
     + 	if (flags & CLEANUP_MSG)
     + 		cleanup = COMMIT_MSG_CLEANUP_ALL;
     ++	else if (flags & VERBATIM_MSG)
     ++		cleanup = COMMIT_MSG_CLEANUP_NONE;
     + 	else if ((opts->signoff || opts->record_origin) &&
     + 		 !opts->explicit_cleanup)
     + 		cleanup = COMMIT_MSG_CLEANUP_SPACE;
      @@ sequencer.c: static int do_pick_commit(struct repository *r,
     - 		flags |= AMEND_MSG;
       		if (!final_fixup)
       			msg_file = rebase_path_squash_msg();
     --		else if (file_exists(rebase_path_fixup_msg())) {
     + 		else if (file_exists(rebase_path_fixup_msg())) {
      -			flags |= CLEANUP_MSG;
     -+		else if (file_exists(rebase_path_fixup_msg()))
     ++			flags |= VERBATIM_MSG;
       			msg_file = rebase_path_fixup_msg();
     --		} else {
     -+		else {
     + 		} else {
       			const char *dest = git_path_squash_msg(r);
     - 			unlink(dest);
     - 			if (copy_file(dest, rebase_path_squash_msg(), 0666))
      
       ## t/t3415-rebase-autosquash.sh ##
      @@ t/t3415-rebase-autosquash.sh: test_expect_success 'fixup a fixup' '
     @@ t/t3415-rebase-autosquash.sh: test_expect_success 'fixup a fixup' '
      +	oneline="#818" &&
      +	git commit --allow-empty -m "$oneline" &&
      +	git commit --fixup HEAD --allow-empty &&
     -+	git rebase -ki --autosquash HEAD~2 &&
     ++	git -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&
      +	test "$oneline" = "$(git show -s --format=%s)"
      +'
      +


 sequencer.c                  | 7 ++++++-
 t/t3415-rebase-autosquash.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 8909a467700..092e7b811f0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -943,6 +943,7 @@ N_("you have staged changes in your working tree\n"
 #define CLEANUP_MSG (1<<3)
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
+#define VERBATIM_MSG (1<<6)
 
 static int run_command_silent_on_success(struct child_process *cmd)
 {
@@ -1012,6 +1013,8 @@ static int run_git_commit(const char *defmsg,
 		strvec_pushl(&cmd.args, "-C", "HEAD", NULL);
 	if ((flags & CLEANUP_MSG))
 		strvec_push(&cmd.args, "--cleanup=strip");
+	if ((flags & VERBATIM_MSG))
+		strvec_push(&cmd.args, "--cleanup=verbatim");
 	if ((flags & EDIT_MSG))
 		strvec_push(&cmd.args, "-e");
 	else if (!(flags & CLEANUP_MSG) &&
@@ -1454,6 +1457,8 @@ static int try_to_commit(struct repository *r,
 
 	if (flags & CLEANUP_MSG)
 		cleanup = COMMIT_MSG_CLEANUP_ALL;
+	else if (flags & VERBATIM_MSG)
+		cleanup = COMMIT_MSG_CLEANUP_NONE;
 	else if ((opts->signoff || opts->record_origin) &&
 		 !opts->explicit_cleanup)
 		cleanup = COMMIT_MSG_CLEANUP_SPACE;
@@ -2002,7 +2007,7 @@ static int do_pick_commit(struct repository *r,
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {
-			flags |= CLEANUP_MSG;
+			flags |= VERBATIM_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path_squash_msg(r);
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 7bab6000dc7..88040bc4352 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '
 	test XZWY = $(git show | tr -cd W-Z)
 '
 
+test_expect_success 'fixup does not clean up commit message' '
+	oneline="#818" &&
+	git commit --allow-empty -m "$oneline" &&
+	git commit --fixup HEAD --allow-empty &&
+	git -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
 test_done

base-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains
  2021-01-08 16:28 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
@ 2021-01-12 20:49   ` Martin Ågren
  2021-01-12 23:12   ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Martin Ågren @ 2021-01-12 20:49 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Vojtěch Knyttl, SZEDER Gábor, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains
  2021-01-08 16:28 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
  2021-01-12 20:49   ` Martin Ågren
@ 2021-01-12 23:12   ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-01-12 23:12 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Vojtěch Knyttl, SZEDER Gábor, Martin Ågren,
	Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

Good thinking.

> Reported-by: Vojtěch Knyttl <vojtech@knyt.tl>
> Helped-by: Martin Ågren <martin.agren@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> ...
> diff --git a/sequencer.c b/sequencer.c
> index 8909a467700..092e7b811f0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -943,6 +943,7 @@ N_("you have staged changes in your working tree\n"
>  #define CLEANUP_MSG (1<<3)
>  #define VERIFY_MSG  (1<<4)
>  #define CREATE_ROOT_COMMIT (1<<5)
> +#define VERBATIM_MSG (1<<6)

It somewhat bothers me that these pretend to be orthogonal options
that can be mixed and matched, but CLEANUP and VERBATIM do not make
sense to be used at the same time.  As long as we have some safety
to ensure that both bits are not used at the same time, i.e.e.g.

	if ((flags & (CLEANUP_MSG|VERBATIM_MSG)) == (CLEANUP_MSG|VERBATIM_MSG))
		BUG("cleanup and verbatim asked at the same time");

it would be OK, though.

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-01-13  0:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2021-01-12 23:12   ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git