Hi Robie, I think it's great to have best practices for quilt patches.
Since you also mentioned renaming of patches, this raises the question if there is also a best practice for a quilt patch naming scheme? (I'm at least not aware of one.)
I noticed several different ones, just to name a few:
<full-hash>.patch
<name of the upstream patch, spaces replaced by dashes, shortened>.patch
<arbitrary name, different from the upstream patch description description>.patch
lp-<bug number>-<name of the upstream patch, spaces replaced by dashes, shortened>.patch
etc. (I think I also saw '.diff' as suffix instead of '.patch')
What I personally liked most so far, for upstream patches (and adopted since then) is:
<short-hash>-<name of the upstream patch, spaces replaced by dashes, shortened>.patch
But maybe different teams (or individuals) just have different preferences.
And btw. I '+1' with Marc - also ran once into issues in the past, where offsets piled up too much.
So it might be needed - in rare cases. (But understood - only in such rare cases...)
Since you also mentioned renaming of patches, this raises the question if there is also a best practice for a quilt patch naming scheme? (I'm at least not aware of one.)
I noticed several different ones, just to name a few:
<full-hash>.patch
<name of the upstream patch, spaces replaced by dashes, shortened>.patch
<arbitrary name, different from the upstream patch description description>.patch
lp-<bug number>-<name of the upstream patch, spaces replaced by dashes, shortened>.patch
etc. (I think I also saw '.diff' as suffix instead of '.patch')
What I personally liked most so far, for upstream patches (and adopted since then) is:
<short-hash>-<name of the upstream patch, spaces replaced by dashes, shortened>.patch
But maybe different teams (or individuals) just have different preferences.
And btw. I '+1' with Marc - also ran once into issues in the past, where offsets piled up too much.
So it might be needed - in rare cases. (But understood - only in such rare cases...)
Bye, Frank
On Wed, May 25, 2022 at 5:31 PM Marc Deslauriers <marc.deslauriers@canonical.com> wrote:
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
