Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 3652 bytes --]

Hello,

On Wed, Jul 01, 2026 at 11:38:27AM +0200, Uwe Kleine-König wrote:
> On Tue, Jun 30, 2026 at 04:28:50PM +0100, Phillip Wood wrote:
> > On 19/06/2026 11:13, Phillip Wood wrote:
> > > I'm happy to take this forward and try and fix at least some of the
> > > other bugs I've listed above. Uwe - if I don't cc you on some patches
> > > within the next couple of weeks please feel free to send a reminder.
> > 
> > Here is the first batch that fixes the same problem as Uwe's patch. I've
> > taken a slightly different approach that uses the return value from
> > do_pick_commit() to signal that a commit was dropped rather than
> > adding another function argument. That involves a number of preparatory
> > patches, but they are hopefully reasonably small and easy to follow.
> > 
> > If a commit gets dropped because its changes are already upstream
> > then we should not record it as rewritten. As well as confusing any
> > post-rewrite hooks this means we end up copying the notes from the
> > dropped commit to the commit that was picked immediately before the
> > one that was dropped.
> > 
> > This series is structured as follows:
> > 
> > Patch 1 restores some test coverage that was lost when the default
> > rebase backend was changed.
> > 
> > Patch 2 moves a function so it can be called without a forward
> > declaration in Patch 11.
> > 
> > Patches 3 & 4 fix the return value of do_pick_commit() when an external
> > command fails (this is in preparation for patch 10).
> > 
> > Patches 5-9 try and simplify the control flow in pick_one_commit()
> > in preparation for patch 10.
> > 
> > Patch 10 changes the return type of do_pick_commit() to an enum.
> > 
> > Patch 11 adds a new member to the enum from patch 10 for commits that
> > are dropped when they become empty and uses that to stop them from
> > being recorded as rewritten.
> 
> With my very little knowledge about git internals, this looks
> reasonable, and it behaves as I expect in my test case. I installed a
> local 
> 
> Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

While it works fine in my test case, it doesn't in my real-life
workflow.

I have a big branch of changes that I maintain on top of next/master, on
todays rebase I experience:

	uwe@monoceros:~/gsrc/linux-2nd$ git rebase --onto=next-20260717 next-20260716 -r -i device_id^{}
	... handling commits that get empty using `git rebase --skip` ...

	uwe@monoceros:~/gsrc/linux-2nd$ git range-diff next-20260716..device_id next-20260717..
	...
	 24:  901ca5f67bc5 !  24:  9f3e8813f6b4 mtd: nand-omap2: Move omap_nand_ids[] to raw nand driver
	    @@ Commit message
	      ## Notes ##
		 Forwarded: id:901ca5f67bc57219a9222115fabe1a1729b87e25.1784229863.git.ukleinek@kernel.org

	    +    Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com
	    +
	      ## drivers/memory/omap-gpmc.c ##
	     @@ drivers/memory/omap-gpmc.c: static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
			of_property_read_bool(np, "gpmc,time-para-granularity");
	 25:  69be5d4f9f13 <   -:  ------------ drm/radeon: Only define radeon_acpi_vfct_match when actually used
	...

with:

	uwe@monoceros:~/gsrc/linux-2nd$ git notes show 69be5d4f9f13
	Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com

When I rebase without -i, the rebase happens without hitting empty
commits that I have to manually skip and then the notes for 69be5d4f9f13
doesn't make it into the neighbour commit after rebase.

So it seems there is still something fishy with interactive rebase.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]