From: Phillip Wood <phillip.wood123@gmail.com>
To: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>, git@vger.kernel.org
Subject: Re: [PATCH 0/8] sequencer refactoring
Date: Thu, 6 Apr 2023 13:09:12 +0100 [thread overview]
Message-ID: <7663c59d-2bdd-56b2-dab9-3011f9398c09@gmail.com> (raw)
In-Reply-To: <f72b3820-124c-3e2c-30e2-ca3f46b74dc0@dunelm.org.uk>
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(-)
>>
next prev parent reply other threads:[~2023-04-06 12:09 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-03-23 19:40 ` Phillip Wood
2023-03-23 20:00 ` Junio C Hamano
2023-03-23 21:08 ` Felipe Contreras
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-08-15 14:01 ` Phillip Wood
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-10-20 21:51 ` Junio C Hamano
2023-10-20 9:36 ` [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
2023-10-23 15:43 ` Phillip Wood
2023-10-23 19:02 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-03-23 19:39 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 21:27 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
2023-03-23 19:31 ` Phillip Wood
2023-03-23 22:38 ` Oswald Buddenhagen
2023-03-24 14:15 ` Phillip Wood
2023-03-24 14:42 ` Oswald Buddenhagen
2023-03-23 20:16 ` Junio C Hamano
2023-03-23 23:23 ` Oswald Buddenhagen
2023-03-24 4:31 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
2023-03-23 19:46 ` Phillip Wood
2023-03-23 22:13 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
2023-03-23 19:34 ` Phillip Wood
2023-03-23 21:36 ` Oswald Buddenhagen
2023-03-24 14:18 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Oswald Buddenhagen
2023-03-26 14:28 ` Phillip Wood
2023-04-26 15:34 ` Oswald Buddenhagen
2023-05-17 12:13 ` Phillip Wood
2023-08-24 16:46 ` Oswald Buddenhagen
2023-03-23 19:38 ` [PATCH 0/8] sequencer refactoring Phillip Wood
2023-03-25 11:08 ` Phillip Wood
2023-04-06 12:09 ` Phillip Wood [this message]
2023-05-17 13:10 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7663c59d-2bdd-56b2-dab9-3011f9398c09@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=oswald.buddenhagen@gmx.de \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).