Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages
Hi Junio,
Junio C Hamano writes:
Fabian Ruch baf...@gmail.com writes:
The command line used to recreate root commits specifies the
erroneous option `--allow-empty-message`. If the root commit has an
empty log message, the replay of this commit should fail and the
rebase should be interrupted like for any other commit that is on the
to-do list and has an empty commit message. Remove the option.
The option might have been introduced by copy-and-paste of the first
part of the command line which initializes the authorship of the
sentinel commit. Indeed, the sentinel commit has an empty log message
and this should not trigger a failure, which is why the option
`--allow-empty-message` is correctly specified here.
The first commit --amend uses -C $1 to give the amended result
not just the authorship but also the log message taken from $1.
If we are allowing a commit without any message to be used as $1,
I think --allow-empty-message needs to be there. If $1 requires
the option here, why doesn't the second one, that records the
updated tree with the metainformation taken from the same commit
$1 can successfully commit without the option?
(I realize now that the emptiness of the sentinel log message is
irrelevant to the success of the first commit --amend since we are
amending using -C. I'll rewrite the second paragraph of the patch
description.)
The first commit --amend requires --allow-empty-message because we do
not want to fail without the authorship or log message of $1 being in
place. It's not a matter of allowing or disallowing empty log messages yet.
git-rebase--interactive can come across an empty log message in three
different ways, which are, depicted as to-do list tasks, the following.
1) pick --ff $1
2) pick --no-ff $1
3) reword $1
This patch is concerned with consistency in the second case.
git-rebase--root does not handle the first case yet and the third case
is handled somewhere else in the script independent of the first two.
The --root option handling was added to the script as a special case
later in the revision history. It's that option handling which
introduced the inconsistency that non-fast-forwards of commits with
empty log messages succeed if they are root commits but have always
failed otherwise.
Your reply suggests that git-rebase--interactive was wrong from the
beginning and that the replay of commits without any message should be
allowed. This would reconcile the first case with the second. In fact,
since neither of them alters the changes introduced by $1 or its log
message, it might be incorrect to complain about a missing message in
the first place.
Do you want me to replace this patch with a patch
rebase -i: Always allow picking of commits with empty log messages
that makes git-rebase--interactive cherry-pick commits using
--allow-empty-message? The script would still abort an empty reword with
the new patch and the user could then still force the empty log message
with git commit --amend --allow-empty-message.
Fabian
Puzzled...
Add test.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 2 +-
t/t3412-rebase-root.sh | 39 +++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4c875d5..0af96f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -510,7 +510,7 @@ do_pick () {
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1
pick_one -n $1
-git commit --allow-empty --allow-empty-message \
+git commit --allow-empty \
--amend --no-post-rewrite -n -q -C $1 \
${gpg_sign_opt:+$gpg_sign_opt} ||
die_with_patch $1 Could not apply $1... $2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages
Fabian Ruch baf...@gmail.com writes: ... Do you want me to replace this patch with a patch rebase -i: Always allow picking of commits with empty log messages that makes git-rebase--interactive cherry-pick commits using --allow-empty-message? I do not want any particular behaviour change. I wanted you to clarify what changed behaviour you are trying to implement and why, and that was why I was asking these questions. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages
Fabian Ruch baf...@gmail.com writes: Your reply suggests that git-rebase--interactive was wrong from the beginning and that the replay of commits without any message should be allowed. This would reconcile the first case with the second. In fact, since neither of them alters the changes introduced by $1 or its log message, it might be incorrect to complain about a missing message in the first place. ... Do you want me to replace this patch with a patch Now you explained your line of thought more clearly and I have thought about it a bit more, I think I agree with the conclusion of the above. An alternative may be to teach a new option --allow-empty-message to rebase to allow people to replay/reproduce an existing history with commits without any message, and uniformly tighten to refuse replaying a commit without message by default. But I am not sure if that is a good direction to go. Doesn't rebase without -i by default replay a commit without message faithfully? It might be debatable to allow a commit that we would normally would flag as suspicious (i.e. no change relative to its parent, or no log message) when replaying with reword or edit, but when replaying with pick, rebase -i should behave just like rebase without interactive. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 06/19] rebase -i: Stop on root commits with empty log messages
Fabian Ruch baf...@gmail.com writes:
The command line used to recreate root commits specifies the
erroneous option `--allow-empty-message`. If the root commit has an
empty log message, the replay of this commit should fail and the
rebase should be interrupted like for any other commit that is on the
to-do list and has an empty commit message. Remove the option.
The option might have been introduced by copy-and-paste of the first
part of the command line which initializes the authorship of the
sentinel commit. Indeed, the sentinel commit has an empty log message
and this should not trigger a failure, which is why the option
`--allow-empty-message` is correctly specified here.
The first commit --amend uses -C $1 to give the amended result
not just the authorship but also the log message taken from $1.
If we are allowing a commit without any message to be used as $1,
I think --allow-empty-message needs to be there. If $1 requires
the option here, why doesn't the second one, that records the
updated tree with the metainformation taken from the same commit
$1 can successfully commit without the option?
Puzzled...
Add test.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 2 +-
t/t3412-rebase-root.sh | 39 +++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4c875d5..0af96f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -510,7 +510,7 @@ do_pick () {
git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1
pick_one -n $1
- git commit --allow-empty --allow-empty-message \
+ git commit --allow-empty \
--amend --no-post-rewrite -n -q -C $1 \
${gpg_sign_opt:+$gpg_sign_opt} ||
die_with_patch $1 Could not apply $1... $2
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 0b52105..9867705 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict
(second part)' '
test_cmp expect-conflict-p out
'
+test_expect_success 'stop rebase --root on empty root log message' '
+ # create a root commit with a non-empty tree so that rebase does
+ # not fail because of an empty commit, and an empty log message
+ echo root-commit file
+ git add file
+ tree=$(git write-tree)
+ root=$(git commit-tree $tree /dev/null)
+ git checkout -b no-message-root-commit $root
+ # do not ff because otherwise neither the patch nor the message
+ # are looked at and checked for emptiness
+ test_when_finished git rebase --abort
+ test_must_fail env EDITOR=true git rebase -i --force-rebase --root
+ echo root-commit file.expected
+ test_cmp file.expected file
+'
+
+test_expect_success 'stop rebase --root on empty child log message' '
+ # create a root commit with a non-empty tree and provide a log
+ # message so that rebase does not fail until the root commit is
+ # successfully replayed
+ echo root-commit file
+ git add file
+ tree=$(git write-tree)
+ root=$(git commit-tree $tree -m root-commit)
+ git checkout -b no-message-child-commit $root
+ # create a child commit with a non-empty patch so that rebase
+ # does not fail because of an empty commit, but an empty log
+ # message
+ echo child-commit file
+ git add file
+ git commit --allow-empty-message --no-edit
+ # do not ff because otherwise neither the patch nor the message
+ # are looked at and checked for emptiness
+ test_when_finished git rebase --abort
+ test_must_fail env EDITOR=true git rebase -i --force-rebase --root
+ echo child-commit file.expected
+ test_cmp file.expected file
+'
+
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
