On 2022-05-25 10:50, Robie Basak wrote: > Today on my SRU shift I spent some effort trying to see past quilt diff > noise. Most of it seemed unnecessary and was only present because of > gratuitous refreshes. > > I'm not sure we have best practices are documented anywhere. I'd like to > define some guidelines that make reviews easier. I think many people > stick to these already. Can we agree that they're a good thing and try > to get more people to do them? > > Summary > > 1. Use dep3 headers (https://dep-team.pages.debian.net/deps/dep3/) > > 2. Use "-p ab --no-timestamps --no-index" or equivalent (as documented at > https://wiki.debian.org/UsingQuilt#Environment_variables). > > 3. Try not to update or refresh any quilt patch unless specifically > required (ie. a patch doesn't apply, or applies with fuzz). Exception: > do update dep3 headers. > > Details and rationale: > > 1. Use dep3 headers (https://dep-team.pages.debian.net/deps/dep3/) > > This makes it easy for reviewers to correlate the patch against > upstream, find any related upstream commentary or subsequent related > commits, etc. > > 2. Use "-p ab --no-timestamps --no-index" or equivalent (as documented at > https://wiki.debian.org/UsingQuilt#Environment_variables). > > This reduces diff noise in the future if an update becomes required. > > Exception: if taking the patch from upstream and it applies as-is, then > using the upstream form of the patch reduces the initial diff further, > so that's preferable. So this combines with 3: use these settings when a > refresh is required or generating the patch from scratch, but don't > refresh to apply the settings to reduce noise unless actually required. > > 3. Try not to update or refresh any quilt patch unless specifically > required (ie. a patch doesn't apply, or applies with fuzz). Exception: > do update dep3 headers. > > This reduces the size of any diff taken against any other version of the > quilt patch. Hopefully to zero. > > If one patch needs refreshing, refresh just that one rather than all of > them. > > If a patch applies with offset, that's not a reason to refresh it.
I kind of disagree with this, I've hit issues before when the offsets were big enough that adding another patch before one with offsets would result in the patch being applied to the wrong function. While nobody should be refreshing patches for no reason to keep the debdiff changes to a minimum, I think it's reasonable to refresh newly added patches, and I would disagree with any best practice that states the opposite. > > Example 1: you can append ".patch" to an upstream Github URL, download > that to debian/patches/, rename and add dep3 headers but without > modifying the patch itself. Then a reviewer can look at the dep3 > header, identify that the origin was GitHub, diff against that same URL, > and easily confirm that the patch hasn't been modified. > > Example 2: if I'm reviewing an SRU to multiple releases and the quilt > patches are exactly identical, then I only have to review the patch > itself once[1]. But if you've unnecessarily refreshed the patch on each > upload, now I have a bunch of noise I have to review to verify that there > is no functional change introduced. > > Does this sound reasonable to everyone to follow in general, such that > we can document these as guidelines somewhere? I wouldn't expect them to > be enforced as a hard rule, but once documented I also think it'd be > reasonable for any reviewer to choose to push back where non-adherence > is causing an actual problem. > > Anything to change, or anything to add? > > Thanks, > > Robie > > > [1] The context outside the patch itself might be different and I do > have to consider that too, but if I know the patches are identical, that > is also made easier. > > Marc. -- ubuntu-devel mailing list ubuntu-devel@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel
