Re: [PATCH v3 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo'

>>> diff --git a/rebase-interactive.c b/rebase-interactive.c
>>> index ad5dd49c31..80b6a2e7a6 100644
>>> --- a/rebase-interactive.c
>>> +++ b/rebase-interactive.c
>>> @@ -97,7 +97,8 @@ int edit_todo_list(struct repository *r, struct
>>> todo_list *todo_list,
>>>              struct todo_list *new_todo, const char *shortrevisions,
>>>              const char *shortonto, unsigned flags)
>>>   {
>>> -    const char *todo_file = rebase_path_todo();
>>> +    const char *todo_file = rebase_path_todo(),
>>> +        *todo_backup = rebase_path_todo_backup();
>>>       /* If the user is editing the todo list, we first try to parse
>>> @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct
>>> todo_list *todo_list,
>>>                       -1, flags | TODO_LIST_SHORTEN_IDS |
>>> TODO_LIST_APPEND_TODO_HELP))
>>>           return error_errno(_("could not write '%s'"), todo_file);
>>>   -    if (initial && copy_file(rebase_path_todo_backup(), todo_file,
>>> 0666))
>>> -        return error(_("could not copy '%s' to '%s'."), todo_file,
>>> -                 rebase_path_todo_backup());
>>> +    unlink(todo_backup);
>>> +    if (copy_file(todo_backup, todo_file, 0666))
>>> +        return error(_("could not copy '%s' to '%s'."), todo_file,
>>> todo_backup);
>>
>> We used to copy ONLY when initial is set and we left old todo_backup
>> intact when !initial.  That is no longer true after this change, but
>> it is intended---we create an exact copy of what we would hand out
>> to the end-user, so that we can compare it with the edited result
>> to figure out what got changed.
> 
> I think it would be better to only create a new copy if the last edit
> was successful. As it stands if I edit the todo list and accidentally
> delete some lines and then edit the todo list again to try and fix it
> the second edit will succeed whether or not I reinserted the deleted lines.
> 
> We could add this to the tests to check that a subsequent edit that does
> not fix the problem fails
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 969e12d281..8544d8ab2c 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> 
> @@ -1416,6 +1416,7 @@ test_expect_success 'rebase --edit-todo respects
> rebase.missingCommitsCheck = er
>                 test_i18ncmp expect actual &&
>                 test_must_fail git rebase --continue 2>actual &&
>                 test_i18ncmp expect actual &&
> +               test_must_fail git rebase --edit-todo &&
>                 cp orig .git/rebase-merge/git-rebase-todo &&
>                 test_must_fail env FAKE_LINES="1 2 3 4" \
>                         git rebase --edit-todo 2>actual &&
> 
> 

>>
>> We unlink(2) unconditionally because the only effect we want to see
>> here is that todo_backup does not exist before we call copy_file()
>> that wants to do O_CREAT|O_EXCL.  I wonder if we want to avoid
>> unlink() when initial, and also if we want to do unlink_or_warn()
>> when !initial (read: this is just "wondering" without thinking long
>> enough to suggest that doing so would be better)
>>
>>> diff --git a/t/t3404-rebase-interactive.sh
>>> b/t/t3404-rebase-interactive.sh
>>> index 29a35840ed..9051c1e11d 100755
>>> --- a/t/t3404-rebase-interactive.sh
>>> +++ b/t/t3404-rebase-interactive.sh
>>> @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects
>>> rebase.missingCommitsCheck = error' '
>>>       test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>>>   '
>>>   +test_expect_success 'rebase --edit-todo respects
>>> rebase.missingCommitsCheck = ignore' '
>>> +    test_config rebase.missingCommitsCheck ignore &&
>>> +    rebase_setup_and_clean missing-commit &&
>>> +    set_fake_editor &&
>>> +    FAKE_LINES="break 1 2 3 4 5" git rebase -i --root &&
>>> +    FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
>>
>> OK, so we lost "pick 5" but with missing-check disabled, that should
>> not trigger any annoying warning or error.
>>
>>> +    git rebase --continue 2>actual &&
> 
> This clobbers actual which hasn't been used yet
> 

>>> +    test D = $(git cat-file commit HEAD | sed -ne \$p) &&
>>
>>> +    test_i18ngrep \
>>> +        "Successfully rebased and updated refs/heads/missing-commit" \
>>> +        actual
>>> +'
>>> +
>>> +test_expect_success 'rebase --edit-todo respects
>>> rebase.missingCommitsCheck = warn' '
>>> +    cat >expect <<-EOF &&
>>> +    error: invalid line 1: badcmd $(git rev-list --pretty=oneline
>>> --abbrev-commit -1 master~4)
>>> +    Warning: some commits may have been dropped accidentally.
>>> +    Dropped commits (newer to older):
>>> +     - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
>>> +    To avoid this message, use "drop" to explicitly remove a commit.
>>> +    EOF
>>> +    tail -n4 expect >expect.2 &&
>>> +    test_config rebase.missingCommitsCheck warn &&
>>> +    rebase_setup_and_clean missing-commit &&
>>> +    set_fake_editor &&
>>> +    test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
>>> +        git rebase -i --root &&
>>> +    cp .git/rebase-merge/git-rebase-todo.backup orig &&
>>> +    FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 &&
>>> +    head -n5 actual.2 >actual &&
>>> +    test_i18ncmp expect actual &&
>>
>> OK, so we lost "pick 1" while discarding "bad", and we should notice
>> the lossage?  I see "head -n5" there, which means we are still
>> getting "invalid line 1: badcmd", even though FAKE_LINES now got rid
>> of "bad"?  Puzzled...
> 
> Is the bad there to stop the rebase so we can edit the todo list? If so
> it would be better to use 'break' instead.
>