Re: [RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages
Hi Eric,
On 06/21/2014 02:33 AM, Eric Sunshine wrote:
> On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch wrote:
>> When `rebase` is executed with `--root` but no `--onto` is specified,
>> `rebase` creates a sentinel commit which is replaced with the root
>> commit in three steps. This combination of options results never in a
>> fast-forward.
>>
>> 1. The sentinel commit is forced into the authorship of the root
>> commit.
>>
>> 2. The changes introduced by the root commit are applied to the index
>> but not committed. If this step fails for whatever reason, all
>> commit information will be there and the user can safely run
>> `git-commit --amend` after resolving the problems.
>>
>> 3. The new root commit is created by squashing the changes into the
>> sentinel commit which already carries the authorship of the
>> cherry-picked root commit.
>>
>> The command line used to create the commit in the third step specifies
>> effectless and erroneous options. Remove those.
>>
>> - `--allow-empty-message` is erroneous: If the root's commit message is
>>empty, the rebase shall fail like for any other commit that is on the
>>to-do list and has an empty commit message.
>>
>>Fix the bug that git-rebase does not fail when the initial commit has
>>an empty log message but is replayed using `--root` is specified.
>>Add test.
>>
>> - `-C` is effectless: The commit being amended, which is the sentinel
>>commit, already carries the authorship and log message of the
>>cherry-picked root commit. The committer email and commit date fields
>>are reset either way.
>>
>> After all, if step two fails, `rebase --continue` won't include these
>> flags in the git-commit command line either.
>>
>> Signed-off-by: Fabian Ruch
>> ---
>> git-rebase--interactive.sh | 4 ++--
>> t/t3412-rebase-root.sh | 39 +++
>> 2 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index fffdfa5..f09eeae 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -539,8 +539,8 @@ 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 \
>> - --amend --no-post-rewrite -n -q -C $1 \
>> + git commit --allow-empty --amend \
>> + --no-post-rewrite -n -q \
>>${gpg_sign_opt:+"$gpg_sign_opt"} ||
>> die_with_patch $1 "Could not apply $1... $2"
>> else
>> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
>> index 0b52105..3608db4 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 > + 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_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
>> + echo root-commit >file.expected &&
>> + test_cmp file file.expected &&
>
> It is customary, and provides nicer diagnostic output upon failure, to
> have the "expected" file mentioned first:
>
> test_cmp file.expected file &&
Ok.
>> + git rebase --abort
>
> Do you want to place this under control of test_when_finished
> somewhere above the "git rebase" invocation to ensure cleanup if
> something fails before this point?
Thanks a lot for the hint, I will place the test_when_finished directly
above the rebase -i command line because it is not relevant before, the
exact position shouldn't matter though. I didn't know that test_run_
skips the cleanup code if -i (for immediate mode) is passed to the test
driver and the test case fails.
Fabian
>> +'
>> +
>> +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-mess
Re: [RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages
On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch wrote:
> When `rebase` is executed with `--root` but no `--onto` is specified,
> `rebase` creates a sentinel commit which is replaced with the root
> commit in three steps. This combination of options results never in a
> fast-forward.
>
> 1. The sentinel commit is forced into the authorship of the root
> commit.
>
> 2. The changes introduced by the root commit are applied to the index
> but not committed. If this step fails for whatever reason, all
> commit information will be there and the user can safely run
> `git-commit --amend` after resolving the problems.
>
> 3. The new root commit is created by squashing the changes into the
> sentinel commit which already carries the authorship of the
> cherry-picked root commit.
>
> The command line used to create the commit in the third step specifies
> effectless and erroneous options. Remove those.
>
> - `--allow-empty-message` is erroneous: If the root's commit message is
>empty, the rebase shall fail like for any other commit that is on the
>to-do list and has an empty commit message.
>
>Fix the bug that git-rebase does not fail when the initial commit has
>an empty log message but is replayed using `--root` is specified.
>Add test.
>
> - `-C` is effectless: The commit being amended, which is the sentinel
>commit, already carries the authorship and log message of the
>cherry-picked root commit. The committer email and commit date fields
>are reset either way.
>
> After all, if step two fails, `rebase --continue` won't include these
> flags in the git-commit command line either.
>
> Signed-off-by: Fabian Ruch
> ---
> git-rebase--interactive.sh | 4 ++--
> t/t3412-rebase-root.sh | 39 +++
> 2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index fffdfa5..f09eeae 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -539,8 +539,8 @@ 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 \
> - --amend --no-post-rewrite -n -q -C $1 \
> + git commit --allow-empty --amend \
> + --no-post-rewrite -n -q \
>${gpg_sign_opt:+"$gpg_sign_opt"} ||
> die_with_patch $1 "Could not apply $1... $2"
> else
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 0b52105..3608db4 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 + 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_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
> + echo root-commit >file.expected &&
> + test_cmp file file.expected &&
It is customary, and provides nicer diagnostic output upon failure, to
have the "expected" file mentioned first:
test_cmp file.expected file &&
> + git rebase --abort
Do you want to place this under control of test_when_finished
somewhere above the "git rebase" invocation to ensure cleanup if
something fails before this point?
> +'
> +
> +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_must_fail env EDITOR=true git rebase -i --force-reb
