git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Jan 10, 2020, 17:13 UTC
Message-ID
<79ad2e01-2fc3-cd52-7879-5a81370628ef@gmail.com>
In-Reply-To
<0bf602bb-eff0-12f1-fa6c-c670a12f2cee@gmail.com>
Hi Alban
On 09/01/2020 21:13, Alban Gruin wrote:
Show 78 quoted lines
> Hi Phillip,
> 
> Le 09/12/2019 à 17:00, Phillip Wood a écrit :
>>>> 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 &&
>>
>>
> 
> In which case, if the check did not pass at the previous edit, the new
> todo list should be compared to the backup.  As sequencer_continue()
> already does this, extract this to its own function in
> rebase-interactive.c.  To keep track of this, a file is created on the
> disk (as you suggested in your other email.)  At the next edit, if this
> file exists and no errors were found, it is deleted.  The backup is only
> created if there is no errors in `todo_list' and in `new_todo'.
> 
> This would guarantee that there is no errors in the backup, and that the
> edited list is always compared to a list exempt of errors.
> 
> This approach also has the benefit to detect if a commit part of a
> badcmd was dropped.
> 
> After some tweaks (ie. `expect' now lists 2 commits instead of one),
> this passes the test with the change you suggested, and the one you sent
> in your other email.

That sounds good. I'm not sure how it passes the test in my other email though, if sequencer_continue() compares the todo list to the backup wont it still fail when continuing after conflicts as the backup is out of date?

Best Wishes
Phillip
Show 82 quoted lines
>>>
>>> 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
>>
> 
> Good catch.
> 
>>>> +    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.
>>
> 
> No, it was here to show that we can detect dropped commits, even if the
> todo list has an error.
> 
>> Best Wishes
>>
>> Phillip
> 
> Cheers,
> Alban
>
Previous: Alban GruinNext: Alban Gruin
Message 36 of 57 in “rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.”
  1. 0/9 rebase -i: extend rebase.missingCommitsCheck to `--edit-todo' and co.Alban Gruin, Jul 17, 2019
  2. 1/9 t3404: demonstrate that --edit-todo does not check for dropped commitsAlban Gruin, Jul 17, 2019
  3. Junio C HamanoJul 18, 2019
  4. Alban GruinJul 19, 2019
  5. Junio C HamanoJul 19, 2019
  6. 2/9 t3429: demonstrate that rebase exec does not check for dropped commitsAlban Gruin, Jul 17, 2019
  7. 3/9 sequencer: update `total_nr' when adding an item to a todo listAlban Gruin, Jul 17, 2019
  8. Junio C HamanoJul 18, 2019
  9. Alban GruinJul 19, 2019
  10. 4/9 sequencer: update `done_nr' when skipping commands in a todo listAlban Gruin, Jul 17, 2019
  11. Junio C HamanoJul 18, 2019
  12. Alban GruinJul 19, 2019
  13. 5/9 sequencer: move the code writing total_nr on the disk to a new functionAlban Gruin, Jul 17, 2019
  14. Junio C HamanoJul 18, 2019
  15. Alban GruinJul 19, 2019
  16. 6/9 sequencer: add a parameter to sequencer_continue() to accept a todo listAlban Gruin, Jul 17, 2019
  17. 7/9 rebase-interactive: todo_list_check() also uses the done listAlban Gruin, Jul 17, 2019
  18. 8/9 rebase-interactive: warn if commit is dropped with --edit-todoAlban Gruin, Jul 17, 2019
  19. 9/9 sequencer: have read_populate_todo() check for dropped commitsAlban Gruin, Jul 17, 2019
  20. Phillip WoodJul 24, 2019
  21. Johannes SchindelinJul 25, 2019
  22. Alban GruinJul 25, 2019
  23. Phillip WoodJul 29, 2019
  24. Alban GruinSep 24, 2019
  25. 0/2 rebase -i: extend rebase.missingCommitsCheck to `--edit-todo'Alban Gruin, Nov 4, 2019
  26. 1/2 t3404: demonstrate that --edit-todo does not check for dropped commitsAlban Gruin, Nov 4, 2019
  27. 2/2 rebase-interactive: warn if commit is dropped with --edit-todoAlban Gruin, Nov 4, 2019
  28. Phillip WoodNov 5, 2019
  29. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Dec 2, 2019
  30. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Dec 2, 2019
  31. Johannes SchindelinDec 6, 2019
  32. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Dec 2, 2019
  33. Junio C HamanoDec 4, 2019
  34. Phillip WoodDec 9, 2019
  35. Alban GruinJan 9, 2020
  36. Phillip WoodJan 10, 2020
  37. Alban GruinJan 10, 2020
  38. Phillip WoodJan 11, 2020
  39. Phillip WoodDec 9, 2019
  40. Junio C HamanoDec 4, 2019
  41. Alban GruinDec 5, 2019
  42. Phillip WoodDec 6, 2019
  43. Johannes SchindelinDec 6, 2019
  44. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Jan 11, 2020
  45. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Jan 11, 2020
  46. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Jan 11, 2020
  47. Phillip WoodJan 19, 2020
  48. Alban GruinJan 25, 2020
  49. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Jan 25, 2020
  50. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Jan 25, 2020
  51. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Jan 25, 2020
  52. Johannes SchindelinJan 26, 2020
  53. Alban GruinJan 27, 2020
  54. Johannes SchindelinJan 28, 2020
  55. 0/2 rebase -i: extend rebase.missingCommitsCheckAlban Gruin, Jan 28, 2020
  56. 1/2 sequencer: move check_todo_list_from_file() to rebase-interactive.cAlban Gruin, Jan 28, 2020
  57. 2/2 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Alban Gruin, Jan 28, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.