From: Phillip Wood Date: Thu, 06 Apr 2023 12:09:12 GMT Subject: Re: [PATCH 0/8] sequencer refactoring Message-ID: <7663c59d-2bdd-56b2-dab9-3011f9398c09@gmail.com> In-Reply-To: On 25/03/2023 11:08, Phillip Wood wrote: > Hi Oswald > > On 23/03/2023 16:22, Oswald Buddenhagen wrote: >> This is a preparatory series for the separately posted 'rebase >> --rewind' patch, >> but I think it has value in itself. > > I had a hard time applying these patches. In the end I had success with > checking out next, applying > https://lore.kernel.org/git/20230323162234.995514-1-oswald.buddenhagen@gmx.de and then applying this series. It is very helpful to detail the base commit in the cover letter. A series like this should normally be based on master see Documentation/SubmittingPatches. > > Having applied the patches I'm unable to compile them with DEVELOPER=1 > (see Documentation/CodingGuidelines) > > In file included from log-tree.c:20: > sequencer.h:7:6: error: ISO C forbids forward references to ‘enum’ types > [-Werror=pedantic] >     7 | enum rebase_action; >       |      ^~~~~~~~~~~~~ > sequencer.h:140:34: error: ISO C forbids forward references to ‘enum’ > types [-Werror=pedantic] >   140 |                             enum rebase_action action); >       |                                  ^~~~~~~~~~~~~ > sequencer.h:196:26: error: ISO C forbids forward references to ‘enum’ > types [-Werror=pedantic] >   196 |                     enum rebase_action action); >       |                          ^~~~~~~~~~~~~ > > In file included from ./cache.h:12, >                  from ./builtin.h:6, >                  from builtin/rebase.c:8: > builtin/rebase.c: In function ‘cmd_rebase’: > builtin/rebase.c:1246:95: error: left-hand operand of comma expression > has no effect [-Werror=unused-value] >  1246 | (BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST), >       |                               ^ > ./trace2.h:158:69: note: in definition of macro ‘trace2_cmd_mode’ >   158 | #define trace2_cmd_mode(sv) trace2_cmd_mode_fl(__FILE__, > __LINE__, (sv)) >       |     ^~ I think the errors above are best addressed by dropping patch 3 as I don't think the benefit is worth the churn. You say that the existing code is fragile but it is not that hard to follow and is battle tested and known to work. If you need to change things to support --rewind then it would be better to do so in a series that adds that option. > sequencer.c: In function ‘todo_list_rearrange_squash’: > sequencer.c:6346:23: error: operation on ‘items’ may be undefined > [-Werror=sequence-point] >  6346 |                 items = ALLOC_ARRAY(items, todo_list->nr); This is easily fixed by deleting "items =" as ALLOC_ARRAY() does the assignment for us. After dropping patches 3 and 7 and fixing the ARROC_ARRAY() above all the rebase tests pass for each commit and the CI passes - https://github.com/phillipwood/git/actions/runs/4627831184 Best Wishes Phillip > > Best Wishes > > Phillip > >> >> Oswald Buddenhagen (8): >>    rebase: simplify code related to imply_merge() >>    rebase: move parse_opt_keep_empty() down >>    sequencer: pass around rebase action explicitly >>    sequencer: create enum for edit_todo_list() return value >>    rebase: preserve interactive todo file on checkout failure >>    sequencer: simplify allocation of result array in >>      todo_list_rearrange_squash() >>    sequencer: pass `onto` to complete_action() as object-id >>    rebase: improve resumption from incorrect initial todo list >> >>   builtin/rebase.c              |  63 +++++++-------- >>   builtin/revert.c              |   3 +- >>   rebase-interactive.c          |  36 ++++----- >>   rebase-interactive.h          |  27 ++++++- >>   sequencer.c                   | 139 +++++++++++++++++++--------------- >>   sequencer.h                   |  15 ++-- >>   t/t3404-rebase-interactive.sh |  34 ++++++++- >>   7 files changed, 196 insertions(+), 121 deletions(-) >>