| Message ID | 20230323162235.995574-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | sequencer refactoring | expand |
Hi Oswald Thanks for working on this. I've only had time for a quick read of the first 7 patches but there are some worthwhile clean ups here and the series is well structured. I'll try and have a thorough look at the last patch but I'm going to be off line next week so it may take a while. Best Wishes Phillip 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. > > > 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(-) >
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)) | ^~ 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); 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(-) >
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(-) >>
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. When you re-roll this I think it would be worth splitting it into two separate series. Patches 1, 2, 4 & 6 are simple clean ups which don't need much work beyond making sure that (a) the commit messages have a good explanation of the reason for the change (try "git log --author "Jeff King" for examples of good commit messages) and (b) the code follows our coding guidelines (mostly no '//' comments if I remember correctly). Patches 5 & 8 address real problems but are more involved and it will take more time to consider the UI changes and get them right. I'd rather we dropped patches 3 & 7. 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(-) >
This is a preparatory series for the separately posted 'rebase --rewind' patch, but I think it has value in itself. 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(-)