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@xxxxxx 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(-)
